On Thu, Mar 25, 2021 at 2:10 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> On Wed, Mar 24, 2021 at 02:35:56PM -0300, Jason Gunthorpe wrote:
> > 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.
>
> Yeah, I'll update it.
>
> Thanks
>
Yes, making it an exported symbol will help. It needs radical changes
in the driver load/unload path. Let me as well take this feedback to
my internal team .
> >
> > Jason
--
-Regards
Devesh
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
