Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Joshua Thompson <[email protected]>
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
Changed since v1:
- Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
- Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
and IRQF_SHARED makes no difference in this case.
- Removed redundant release_mem_region() call.
- Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
patch (as this patch has some unrelated benefits).
---
arch/m68k/configs/mac_defconfig | 1 +
arch/m68k/mac/config.c | 41 ++++++++++++++++++++
drivers/ide/macide.c | 66 +++++++++++++++++++++------------
3 files changed, 85 insertions(+), 23 deletions(-)
diff --git a/arch/m68k/configs/mac_defconfig b/arch/m68k/configs/mac_defconfig
index 6087798662601..f770970fe4e99 100644
--- a/arch/m68k/configs/mac_defconfig
+++ b/arch/m68k/configs/mac_defconfig
@@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_MAC_IDE=y
CONFIG_RAID_ATTRS=m
CONFIG_SCSI=y
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..43fc29180cb58 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
#include <linux/init.h>
#include <linux/vt_kern.h>
#include <linux/platform_device.h>
+#include <linux/ata_platform.h>
#include <linux/adb.h>
#include <linux/cuda.h>
#include <linux/pmu.h>
@@ -940,6 +941,26 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_F),
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_C),
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x38),
+ DEFINE_RES_MEM(0x50F1A038, 0x04),
+ DEFINE_RES_IRQ(IRQ_BABOON_1),
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
+
int __init mac_platform_init(void)
{
phys_addr_t swim_base = 0;
@@ -1048,6 +1069,26 @@ int __init mac_platform_init(void)
break;
}
+ /*
+ * IDE device
+ */
+
+ switch (macintosh_config->ide_type) {
+ case MAC_IDE_QUADRA:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+ break;
+ case MAC_IDE_PB:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+ break;
+ case MAC_IDE_BABOON:
+ platform_device_register_resndata(NULL, "pata_platform", -1,
+ mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+ &mac_pata_baboon_data, sizeof(mac_pata_baboon_data));
+ break;
+ }
+
/*
* Ethernet device
*/
diff --git a/drivers/ide/macide.c b/drivers/ide/macide.c
index 3c6bb8599303b..f2236456b3706 100644
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
#include <linux/delay.h>
#include <linux/ide.h>
#include <linux/module.h>
+#include <linux/platform_device.h>
#include <asm/macintosh.h>
-#include <asm/macints.h>
-#include <asm/mac_baboon.h>
+
+#define DRV_NAME "mac_ide"
#define IDE_BASE 0x50F1A000 /* Base address of IDE controller */
@@ -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)
{
- unsigned long base;
- int irq;
+ struct resource *mem, *irq;
struct ide_hw hw, *hws[] = { &hw };
struct ide_port_info d = macide_port_info;
+ struct ide_host *host;
+ int rc;
if (!MACH_IS_MAC)
return -ENODEV;
- switch (macintosh_config->ide_type) {
- case MAC_IDE_QUADRA:
- base = IDE_BASE;
- irq = IRQ_NUBUS_F;
- break;
- case MAC_IDE_PB:
- base = IDE_BASE;
- irq = IRQ_NUBUS_C;
- break;
- case MAC_IDE_BABOON:
- base = BABOON_BASE;
- d.port_ops = NULL;
- irq = IRQ_BABOON_1;
- break;
- default:
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!irq)
return -ENODEV;
+
+ if (!devm_request_mem_region(&pdev->dev, mem->start,
+ resource_size(mem), DRV_NAME)) {
+ dev_err(&pdev->dev, "resources busy\n");
+ return -EBUSY;
}
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);
+ return 0;
}
-module_init(macide_init);
+static int mac_ide_remove(struct platform_device *pdev)
+{
+ struct ide_host *host = dev_get_drvdata(&pdev->dev);
+
+ ide_host_remove(host);
+ return 0;
+}
+
+static struct platform_driver mac_ide_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ },
+ .probe = mac_ide_probe,
+ .remove = mac_ide_remove,
+};
+
+module_platform_driver(mac_ide_driver);
+MODULE_ALIAS("platform:" DRV_NAME);
MODULE_LICENSE("GPL");
--
2.26.2
Hi Finn,
On Mon, Sep 14, 2020 at 4:47 AM Finn Thain <[email protected]> wrote:
> Add platform devices for the Mac IDE controller variants. Convert the
> macide module into a platform driver to support two of those variants.
> For the third, use a generic "pata_platform" driver instead.
> This enables automatic loading of the appropriate module and begins
> the process of replacing the driver with libata alternatives.
>
> Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Joshua Thompson <[email protected]>
> References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
> References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
> Tested-by: Stan Johnson <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
> ---
> This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
> using both pata_platform and ide_platform drivers.
> The next step will be to try using these generic drivers with the other
> IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
> driver can be entirely replaced with libata drivers.
>
> Changed since v1:
> - Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
> - Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
> and IRQF_SHARED makes no difference in this case.
> - Removed redundant release_mem_region() call.
> - Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
> CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
> patch (as this patch has some unrelated benefits).
Thanks for the update!
BTW, who do you envision taking this (or the next version)? IDE or m68k?
> --- a/arch/m68k/configs/mac_defconfig
> +++ b/arch/m68k/configs/mac_defconfig
> @@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
> CONFIG_IDE=y
> CONFIG_IDE_GD_ATAPI=y
> CONFIG_BLK_DEV_IDECD=y
> +CONFIG_BLK_DEV_PLATFORM=y
I guess we want this in multi_defconfig, too?
> CONFIG_BLK_DEV_MAC_IDE=y
> CONFIG_RAID_ATTRS=m
> CONFIG_SCSI=y
> --- 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?
> + return 0;
> }
>
> -module_init(macide_init);
> +static int mac_ide_remove(struct platform_device *pdev)
> +{
> + struct ide_host *host = dev_get_drvdata(&pdev->dev);
As you use platform_set_drvdata() above, you might as well use the
platform_get_drvdata() helper here, for symmetry.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 14 Sep 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Mon, Sep 14, 2020 at 4:47 AM Finn Thain <[email protected]> wrote:
> > Add platform devices for the Mac IDE controller variants. Convert the
> > macide module into a platform driver to support two of those variants.
> > For the third, use a generic "pata_platform" driver instead.
> > This enables automatic loading of the appropriate module and begins
> > the process of replacing the driver with libata alternatives.
> >
> > Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: Joshua Thompson <[email protected]>
> > References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
> > References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
> > Tested-by: Stan Johnson <[email protected]>
> > Signed-off-by: Finn Thain <[email protected]>
> > ---
> > This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
> > using both pata_platform and ide_platform drivers.
> > The next step will be to try using these generic drivers with the other
> > IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
> > driver can be entirely replaced with libata drivers.
> >
> > Changed since v1:
> > - Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
> > - Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
> > and IRQF_SHARED makes no difference in this case.
> > - Removed redundant release_mem_region() call.
> > - Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
> > CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
> > patch (as this patch has some unrelated benefits).
>
> Thanks for the update!
>
> BTW, who do you envision taking this (or the next version)? IDE or m68k?
>
It's unclear from the diff stat. But the focus is on the platform which
probably means it is more interesting to you, as the arch maintainer.
> > --- a/arch/m68k/configs/mac_defconfig
> > +++ b/arch/m68k/configs/mac_defconfig
> > @@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
> > CONFIG_IDE=y
> > CONFIG_IDE_GD_ATAPI=y
> > CONFIG_BLK_DEV_IDECD=y
> > +CONFIG_BLK_DEV_PLATFORM=y
>
> I guess we want this in multi_defconfig, too?
>
Right. I'll add that in v3.
> > CONFIG_BLK_DEV_MAC_IDE=y
> > CONFIG_RAID_ATTRS=m
> > CONFIG_SCSI=y
>
> > --- 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.
> > + return 0;
> > }
> >
> > -module_init(macide_init);
> > +static int mac_ide_remove(struct platform_device *pdev)
> > +{
> > + struct ide_host *host = dev_get_drvdata(&pdev->dev);
>
> As you use platform_set_drvdata() above, you might as well use the
> platform_get_drvdata() helper here, for symmetry.
>
Will do.
Thanks for your review.
> Gr{oetje,eeting}s,
>
> Geert
Hi Finn,
On Tue, Sep 15, 2020 at 3:17 AM Finn Thain <[email protected]> wrote:
> On Mon, 14 Sep 2020, Geert Uytterhoeven wrote:
> > On Mon, Sep 14, 2020 at 4:47 AM Finn Thain <[email protected]> wrote:
> > > Add platform devices for the Mac IDE controller variants. Convert the
> > > macide module into a platform driver to support two of those variants.
> > > For the third, use a generic "pata_platform" driver instead.
> > > This enables automatic loading of the appropriate module and begins
> > > the process of replacing the driver with libata alternatives.
> > >
> > > Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> > > Cc: Geert Uytterhoeven <[email protected]>
> > > Cc: Joshua Thompson <[email protected]>
> > > References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
> > > References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
> > > Tested-by: Stan Johnson <[email protected]>
> > > Signed-off-by: Finn Thain <[email protected]>
> > > ---
> > > This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
> > > using both pata_platform and ide_platform drivers.
> > > The next step will be to try using these generic drivers with the other
> > > IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
> > > driver can be entirely replaced with libata drivers.
> > >
> > > Changed since v1:
> > > - Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
> > > - Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
> > > and IRQF_SHARED makes no difference in this case.
> > > - Removed redundant release_mem_region() call.
> > > - Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
> > > CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
> > > patch (as this patch has some unrelated benefits).
> >
> > Thanks for the update!
> >
> > BTW, who do you envision taking this (or the next version)? IDE or m68k?
> >
>
> It's unclear from the diff stat. But the focus is on the platform which
> probably means it is more interesting to you, as the arch maintainer.
Fair enough.
Looking for an Acked-by from the IDE maintainer...
> > > --- 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 :-(
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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.
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"?
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);
Hi Finn,
On 24/09/20 1:07 PM, Finn Thain wrote:
> 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.
Please do - that is clearly a bug. I must admit I never tried to boot my
Falcon off a SCSI partition to test falconide module unload.
Cheers,
Michael
>
> 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);
>
Hi Finn,
On Thu, Sep 24, 2020 at 3:07 AM Finn Thain <[email protected]> wrote:
> 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.
I'm fine with keeping this as-is, as it doesn't really matter for this
particular
driver.
> 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.
Please do so. Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds