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

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.

quoted

+               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)