On Sun, 16 Aug 2020 at 20:17, Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
> [...]
> > + retval = hwmon_init(hdev);
> > + if (retval) {
> > + dev_err(&interface->dev, "failed initialising hwmon %s:%d\n",
> > + hdev->config->name, retval);
> > + kfree(hdev);
> > + kfree(hdev->bulk_in_buffer);
> > + kfree(hdev->bulk_out_buffer);
>
> I hope what I said wasn't confusing, but - like you said - if you use
> devm_* you don't need to explicitly free it. Furthermore, you access a pointer
> that has already been freed here.
well since I didn't put the device it wouldn't have been freed. This
in itself was a
mistake of course since I got the device I should put it as well.
> devm_k.alloc() and devm_kfree() OR k.alloc() and kfree(), do not mix the two.
> In case of failure I think it makes sense to free the resources either way, but
> it is not strictly necessary since devm resources will be freed when the device
> is gone.
I couldn't find any clear documentation on this, but since .disconnect
doesn't get
called if probe fails I can just restructure the code so I can call
astk_delete on
errors. This makes everything much cleaner and nicer.
> > + goto exit;
> > + }
> > +
> > + usb_set_intfdata(interface, hdev);
> > + mutex_init(&hdev->io_mutex);
> > +exit:
> > + return retval;
> > +}
> > [...]
>
>
> Please think more about memory management and how it should work in this module.
> Only manage devm resources between "getting" and "putting" the device.
> Don't do it after "putting" or before "getting" the device.
> And please compile and test your code (among other things) before submitting as per
> https://www.kernel.org/doc/html/latest/hwmon/submitting-patches.html
Thanks for the link to the page, I did compile and test the code
before submitting, but I
must have ctrl-z ed or something for it not to compile. I always make
sure everything
works before submitting (at least setting mode, rpm, pwm and reading rpm).
After looking at the link a bit, I will try and write documentation
tomorrow, and see
how the MAINTAINERS file is structured to add myself.
After that I will post another version, if there is anything else let
me know so I can
incorporate it in the patch.
Kind Regards,
Jaap Aarts
