Re: [PATCH v3 3/3] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

Yep, you are right, and as I continue to mull this over, I realize I can 
make it much simpler.

Sorry about missing the changelog entry, as I had written that prior to 
realizing that all of this cleanup was a workaround for the error you 
found in the first iteration:  flipping the null meant I was leaving 
behind devices when I should have been cleaning them up.  So, yeah, I 
think the list can go now.



On 6/28/24 04:41, Jeremy Kerr wrote:
> You can save the list_entry() below by using list_for_each_entry_safe()
> here.
>
> > +               struct net_device *ndev;
> > +               struct mctp_pcc_ndev *mctp_pcc_dev;
> > +
> > +               mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> > +               if (mctp_pcc_dev->acpi_device != adev)
> > +                       continue;
> Wait, you removed the case where you clean up the whole list if adev is
> NULL?
>
> Now this contradicts your section of the changelog:
>
> - Did not change the module init unload function to use the
>    ACPI equivalent as they do not remove all devices prior
>    to unload, leading to dangling references and seg faults.
>
> Does this mean you no longer need to free all instances on unload? In
> which case, that sounds great! that probably also means:
>
>   - all the list is doing is allowing you to find the mctp_pcc_dev from
>     the acpi_device
>
>   - but: you can turn an acpi_device into a mctp_pcc_dev from
>     adev->driver_data (which you're already setting), so you don't need
>     the list
>
>   - then, _remove just becomes something like:
>
>      static void mctp_pcc_driver_remove(struct acpi_device *adev)
>      {
>              struct mctp_pcc_dev *dev = acpi_driver_data(adev);
>
>              pcc_mbox_free_channel(dev->out_chan);
>              pcc_mbox_free_channel(dev->in_chan);
>              if (dev->mdev.dev)
>                      mctp_unregister_netdev(dev->mdev.dev);
>      }
>
>      (but I can't see how dev->mdev.dev can be null, so you may be able
>      to remove the condition too)