Re: [PATCH 2/9] staging: vc04_services: Log using pr_err() when vchiq_state is unset - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Umang Jain <umang.jain@ideasonboard.com>
Cc: linux-staging@lists.linux.dev,
	linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org,
	Stefan Wahren <stefan.wahren@i2se.com>,
	Dan Carpenter <error27@gmail.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Phil Elwell <phil@raspberrypi.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	"Ricardo B . Marliere" <ricardo@marliere.net>
Subject: Re: [PATCH 2/9] staging: vc04_services: Log using pr_err() when vchiq_state is unset
Date: Thu, 23 Nov 2023 13:02:01 +0000	[thread overview]
Message-ID: <2023112312-epic-viscosity-3848@gregkh> (raw)
In-Reply-To: <20231107095156.365492-3-umang.jain@ideasonboard.com>

On Tue, Nov 07, 2023 at 04:51:49AM -0500, Umang Jain wrote:
> In cases, where the global vchiq state is still unset, we cannot log
> to dynamic debug (access to struct device is needed, hence potential
> NULL de-reference). Log using pr_err() instead.

No, something is wrong here, don't do that.

> In vchiq_initialise(), remove the 'goto' because it is just again
> trying to log to dynamic debug. Simply return with -ENNOTCONN after
> logging to pr_err().
> 
> In vchiq_open(), move the vchiq_log_debug() after the state pointer
> null check.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c  | 6 ++----
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c  | 7 +++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 9fb8f657cc78..9fb3e240d9de 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -687,10 +687,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>  		usleep_range(500, 600);
>  	}
>  	if (i == VCHIQ_INIT_RETRIES) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not initialized\n",
> -				__func__);
> -		ret = -ENOTCONN;
> -		goto failed;
> +		pr_err("%s: videocore not initialized\n", __func__);
> +		return -ENOTCONN;

Here's a good reason to get rid of the crazy "this subsystem is special
so let us use a custom logging macro" logic.

Convert everything to just use real dev_*() calls so it makes it obvious
what is happening and how it all is working.  It will save you from
doing stuff like this in the future as you will "know" that there isn't
a valid device pointer here.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel