lkml.org 
[lkml]   [2016]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

On Tue, 27 Sep 2016, Ondrej Zary wrote:

> On Monday 26 September 2016, Finn Thain wrote:
>
> [...]
>
> > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > > Scsi_Host *instance) release_mem_region(instance->base,
> > > hostdata->iomem_size);
> > > }
> > > #endif
> > > - return 0;
> > > + scsi_host_put(instance);
> >
> > The sequence of operations here should be the same as the error path
> > above.
>
> I put scsi_host_put() call at the end because the release_mem_region
> code (in the MMIO case) dereferences the hostdata pointer. I don't think
> it's safe to do after scsi_host_put().

It's funny you say that, I already fixed a few of those use-after-free
bugs in the other 5380 drivers. To avoid the bug, you'd need to use a
temporary variable, like the fixes I did elsewhere.

Don't worry about changing it, this part of the driver gets revisited in
my existing patch queue anyway.

>
> [...]
>
> > > +static int pnp_registered;
> > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > > +
> > > +static int __init generic_NCR5380_init(void)
> > > +{
> > > + int ret = 0;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> > > + if (!ret)
> > > + pnp_registered = 1;
> > > #endif
> > > + ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> > > + if (!ret)
> > > + isa_registered = 1;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + ret = 0;
> > > +#endif
> > > + if (isa_registered)
> > > + ret = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit generic_NCR5380_exit(void)
> > > +{
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> > > +#endif
> > > + if (isa_registered)
> > > + isa_unregister_driver(&generic_NCR5380_isa_driver);
> > > +}
> > > +
> >
> > I find that hard to follow. This should be equivalent and simpler:
> >
> > static int __init generic_NCR5380_init(void)
> > {
> > isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> > return pnp_ret ? isa_ret : 0;
> > #endif
> > return isa_ret;
> > }
> >
> > static void __exit generic_NCR5380_exit(void)
> > {
> > if (!isa_ret)
> > isa_unregister_driver(&generic_NCR5380_isa_driver);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > if (!pnp_ret)
> > pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> > #endif
> > }
>
> Doesn't make it any better, IMHO. Good that it's shorter but not cleaner -
> especially this:
> > return pnp_ret ? isa_ret : 0;

The problem with that line is that pnp_ret is always discarded, same as in
your version. I think my version makes that clear, which is what matters
to me.

>
> Also looking at the _exit function, meaning of isa_ret and pnp_ret is not
> obvious there.
>
> Maybe we should have a module_isa_pnp_driver() macro.
>
>

FWIW I would hope that the hypothetical definition of this
`module_isa_pnp_driver' macro would have the same properties as my
version: shorter, uses no temporaries and uses one less #ifdef, and is
generally easier for me to read.

--

\
 
 \ /
  Last update: 2016-09-28 02:06    [W:0.044 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site