On Wed, Mar 24, 2021 at 10:54:58PM +0530, Devesh Sharma wrote:
> On Wed, Mar 24, 2021 at 10:26 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 24, 2021 at 10:00:05PM +0530, Devesh Sharma wrote:
> >
> > > > > > -static void bnxt_re_dev_unprobe(struct net_device *netdev,
> > > > > > - struct bnxt_en_dev *en_dev)
> > > > > > -{
> > > > > > - dev_put(netdev);
> > > > > > - module_put(en_dev->pdev->driver->driver.owner);
> > > > > > -}
> > > > >
> > > > > And you are right to be wondering WTF is this
> > >
> > > Still trying to understand but what's the big idea here may be I can help.
> >
> > A driver should not have module put things like the above
> >
> > It should not be accessing ->driver without holding the device_lock()
> >
> > Basically it is all nonsense coding, Leon suggests to delete it and he
> > is probably right.
> >
> > Can you explain what it thinks it is doing?
> That F'ed up code is trying to prevent a situation where someone
> tries to remove the bnxt_en driver while bnxt_re driver is using it.
> All because bnxt_re driver is at the mercy of bnxt_en drive and there
> is not symbole dependence, Do you suggest anything to prevent that
> unload of bnxt_en other than doing this jargon.
Well, the module put says nothing about the validity of the 'struct
bnxt' and related it extracted from the netdev - you should have a
mechanism that prevents that from going invalid which in turn will
ensure the function pointers you want to touch are still valid
too. (as the struct containing function pointers must become invalid
before the module unloads)
Probably the netdev refcount does that already but I always forget the
exact point during unregister that it waits on that...
As far as strict module dependencies go, replace the pointless
brp->ulp_probe function pointer with an actual call to
bnxt_ulp_probe() and you get the same effect as the module_get.
Jason
