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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdX1GXMGg1mviE_msM1vx0ZLEKEPT4AacSBjYDzN6sZVmw@mail.gmail.com>
Date:   Tue, 15 Sep 2020 09:15:11 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Finn Thain <fthain@...egraphics.com.au>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Joshua Thompson <funaho@...ai.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-ide@...r.kernel.org
Subject: Re: [PATCH v2] ide/macide: Convert Mac IDE driver to platform driver

Hi Finn,

On Tue, Sep 15, 2020 at 3:17 AM Finn Thain <fthain@...egraphics.com.au> wrote:
> On Mon, 14 Sep 2020, Geert Uytterhoeven wrote:
> > On Mon, Sep 14, 2020 at 4:47 AM Finn Thain <fthain@...egraphics.com.au> 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 <b.zolnierkie@...sung.com>
> > > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > > Cc: Joshua Thompson <funaho@...ai.org>
> > > 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 <userm57@...oo.com>
> > > Signed-off-by: Finn Thain <fthain@...egraphics.com.au>
> > > ---
> > > 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 -- geert@...ux-m68k.org

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