Re: [PATCH net-next v8 1/8] bnxt_en: Add auxiliary driver support

[Ajit Khaparde]

On Mon, Jan 23, 2023 at 10:42 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jan 23, 2023 at 10:33:05PM -0800, Jakub Kicinski wrote:
> > On Thu, 19 Jan 2023 22:05:28 -0800 Ajit Khaparde wrote:
> > > @@ -13212,6 +13214,7 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> > >     kfree(bp->rss_indir_tbl);
> > >     bp->rss_indir_tbl = NULL;
> > >     bnxt_free_port_stats(bp);
> > > +   bnxt_aux_priv_free(bp);
> > >     free_netdev(dev);
> >
> > You're still freeing the memory in which struct device sits regardless
> > of its reference count.
> >
> > Greg, is it legal to call:
> >
> >       auxiliary_device_delete(adev);  // AKA device_del(&auxdev->dev);
> >       auxiliary_device_uninit(adev);  // AKA put_device(&auxdev->dev);
> >       free(adev);                     // frees struct device
>
> Ick, the aux device release callback should be doing the freeing of the
> memory, you shouldn't ever have to free it "manually" like this.  To do
> so would be a problem (i.e. the release callback would then free it
> again, right?)
Yikes!
Thanks for the refresher.

>
> > ? I tried to explain this three times, maybe there's some wait during
> > device_del() I'm not seeing which makes this safe :S
Apologies.
I thought since the driver was allocating the memory, it had to free it.
I stand corrected.

Thanks
Ajit

>
> Nope, no intentional wait normally.  You can add one by enabling a
> debugging option to find all of the places where people are doing bad
> things like this by delaying the freeing of the device memory until a
> few seconds later, but that's generally not something you should run in
> a production kernel as it finds all sorts of nasty bugs...
>
> thanks,
>
> greg k-h

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature