Re: [PATCH v2] ide/macide: Convert Mac IDE driver to platform driver

[Finn Thain]

Hi Geert,

On Wed, 16 Sep 2020, Finn Thain wrote:

> On Tue, 15 Sep 2020, Geert Uytterhoeven wrote:
> 
> > > > > --- a/drivers/ide/macide.c
> > > > > +++ b/drivers/ide/macide.c
> > > >
> > > > > @@ -109,42 +110,61 @@ static const char *mac_ide_name[] =
> > > > >   * Probe for a Macintosh IDE interface
> > > > >   */
> > > > >
> > > > > -static int __init macide_init(void)
> > > > > +static int mac_ide_probe(struct platform_device *pdev)
> > > > >  {
> > > >
> > > > >         printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
> > > > >                          mac_ide_name[macintosh_config->ide_type - 1]);
> > > > >
> > > > > -       macide_setup_ports(&hw, base, irq);
> > > > > +       macide_setup_ports(&hw, mem->start, irq->start);
> > > > >
> > > > > -       return ide_host_add(&d, hws, 1, NULL);
> > > > > +       rc = ide_host_add(&d, hws, 1, &host);
> > > > > +       if (rc)
> > > > > +               return rc;
> > > > > +
> > > > > +       platform_set_drvdata(pdev, host);
> > > >
> > > > Move one up, to play it safe?
> > > >
> > >
> > > You mean, before calling ide_host_add? The 'host' pointer is 
> > > uninitialized prior to that call.
> > 
> > Oh right, so the IDE subsystem doesn't let you use the drvdata inside 
> > your driver (besides in remove()) in a safe way :-(
> > 
> 
> The IDE subsystem does allow other patterns here. I could have changed 
> ide_host_alloc() into ide_host_register() followed by ide_host_add() but 
> I could not see any benefit from that change.
> 

Sorry, I meant to say, "I could have changed ide_host_add() into 
ide_host_alloc() followed by ide_host_register() ..."

> A quick search for "platform_device" shows that the driver does not use 
> any uninitialized driver_data pointer (because ide_ifr is a global). In 
> your message of September 9th you readily reached the same conclusion 
> when you reviewed v1.
> 
> If mac_ide_probe() followed the usual pattern it might make review 
> easier (as reviewers may not wish to consider the entire driver) but 
> does that really make the code more "safe"?
> 

I still think that "if it ain't broke, don't fix it" is actually the 
"safe" option for macide.c. But I'm happy to make additional changes, test 
them and send v5 if that's preferred.

Looking further at the drivers using ide_host_register(), I see that 
falconide.c is missing a set_drvdata() call, while tx4939ide.c calls 
set_drvdata() after ide_host_register(). The latter example is not a bug. 

The pattern I used, that is, calling set_drvdata() after ide_host_add(), 
is actually more popular among IDE drivers than the pattern you suggested, 
that is, set_drvdata() followed by ide_host_register(). Either way, I 
don't see any bugs besides the one in falconide.c.

Regarding falconide.c, my inclination is to send a fix following the more 
common pattern (among IDE drivers), as below. I guess that may prompt the 
subsystem maintainers to make known their views on the style question.

diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c
index dbeb2605e5f6e..607c44bc50f1b 100644
--- a/drivers/ide/falconide.c
+++ b/drivers/ide/falconide.c
@@ -166,6 +166,7 @@ static int __init falconide_init(struct platform_device *pdev)
 	if (rc)
 		goto err_free;
 
+	platform_set_drvdata(pdev, host);
 	return 0;
 err_free:
 	ide_host_free(host);
@@ -176,7 +177,7 @@ static int __init falconide_init(struct platform_device *pdev)
 
 static int falconide_remove(struct platform_device *pdev)
 {
-	struct ide_host *host = dev_get_drvdata(&pdev->dev);
+	struct ide_host *host = platform_get_drvdata(pdev);
 
 	ide_host_remove(host);