Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Dec 2022 14:10:54 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Umang Jain <umang.jain@ideasonboard.com>,
        linux-staging@lists.linux.dev,
        linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Adrien Thierry <athierry@redhat.com>,
        Dan Carpenter <error27@gmail.com>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Phil Elwell <phil@raspberrypi.com>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Kieran Bingham <kieran.bingham@ideasonboard.com>
Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Create
 platform_device per device
Message-ID: <Y6MF3l40WM3onmyO@kroah.com>
References: <20221220084404.19280-1-umang.jain@ideasonboard.com>
 <Y6Lqs8RUiyi452gM@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y6Lqs8RUiyi452gM@pendragon.ideasonboard.com>
Precedence: bulk

On Wed, Dec 21, 2022 at 01:14:59PM +0200, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Tue, Dec 20, 2022 at 02:14:04PM +0530, Umang Jain wrote:
> > Create a proper per device platorm_device structure for all the child
> > devices that needs to be registered by vchiq platform driver. Replace
> > the vchiq_register_child() with platform_add_devices() to register the
> > child devices.
> 
> This explains what the patch does, but not why.
> 
> > This is part of an effort to address TODO item "Get rid of all non
> > essential global structures and create a proper per device structure"
> 
> And this explains part of the reason only. Could you please expand the
> commit message with the reasoning behind this change ? It's not clear
> from the change below why this is needed and good.
> 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           | 59 ++++++++++---------
> >  1 file changed, 31 insertions(+), 28 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 22de23f3af02..fa42ea3791a7 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -65,8 +65,29 @@ int vchiq_susp_log_level = VCHIQ_LOG_ERROR;
> >  DEFINE_SPINLOCK(msg_queue_spinlock);
> >  struct vchiq_state g_state;
> >  
> > -static struct platform_device *bcm2835_camera;
> > -static struct platform_device *bcm2835_audio;
> > +static u64 vchiq_device_dmamask = DMA_BIT_MASK(32);
> 
> The fact that this isn't const and is used by two different
> platform_device instances is worrying. Either it can be made const, or
> it's wrong.
> 
> > +
> > +static struct platform_device bcm2835_camera = {
> > +	.name		= "bcm2835-camera",
> > +	.id		= PLATFORM_DEVID_NONE,
> > +	.dev		= {
> > +		.dma_mask	= &vchiq_device_dmamask,
> > +	}
> > +};
> > +
> > +static struct platform_device bcm2835_audio = {
> > +	.name		= "bcm2835_audio",
> > +	.id		= PLATFORM_DEVID_NONE,
> > +	.dev		= {
> > +		.dma_mask	= &vchiq_device_dmamask,
> > +	}
> > +
> 
> Extra blank line.
> 
> > +};
> > +
> > +static struct platform_device *vchiq_devices[] __initdata = {
> 
> Make it const.
> 
> > +	&bcm2835_camera,
> > +	&bcm2835_audio,
> > +};
> >  
> >  struct vchiq_drvdata {
> >  	const unsigned int cache_line_size;
> > @@ -1763,28 +1784,6 @@ static const struct of_device_id vchiq_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, vchiq_of_match);
> >  
> > -static struct platform_device *
> > -vchiq_register_child(struct platform_device *pdev, const char *name)
> > -{
> > -	struct platform_device_info pdevinfo;
> > -	struct platform_device *child;
> > -
> > -	memset(&pdevinfo, 0, sizeof(pdevinfo));
> > -
> > -	pdevinfo.parent = &pdev->dev;
> > -	pdevinfo.name = name;
> > -	pdevinfo.id = PLATFORM_DEVID_NONE;
> > -	pdevinfo.dma_mask = DMA_BIT_MASK(32);
> > -
> > -	child = platform_device_register_full(&pdevinfo);
> > -	if (IS_ERR(child)) {
> > -		dev_warn(&pdev->dev, "%s not registered\n", name);
> > -		child = NULL;
> > -	}
> > -
> > -	return child;
> > -}
> > -
> >  static int vchiq_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *fw_node;
> > @@ -1832,8 +1831,11 @@ static int vchiq_probe(struct platform_device *pdev)
> >  		goto error_exit;
> >  	}
> >  
> > -	bcm2835_camera = vchiq_register_child(pdev, "bcm2835-camera");
> > -	bcm2835_audio = vchiq_register_child(pdev, "bcm2835_audio");
> > +	err = platform_add_devices(vchiq_devices, ARRAY_SIZE(vchiq_devices));
> > +	if (err) {
> > +		dev_warn(&pdev->dev, "Failed to add vchiq child devices");
> > +		goto error_exit;
> > +	}
> 
> If you unbind and rebind this driver, the platform_device instances
> defined as global variables will be reused, and I'm pretty sure that
> will cause issues, for instance with the kobj->state_initialized check
> in kobject_init() (called from device_initialize(), itself called from
> platform_device_register(), from platform_add_devices()). I'm not sure
> static instances of platform_device are a very good idea in general.

static instances of any device are a horrible idea, but it seems that
many drivers do this and abuse platform devices this way :(

Ideally this should be done properly, with the correct devices created
automatically based on the device tree structure, NOT hard-coded into a
.c file like this.

So I too really do not like this change, why are these not being created
by the firware layer automatically?

thanks,

greg k-h