Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[Jackie Liu <liu.yun@xxxxxxxxx>]

在 2021/12/22 上午10:58, Guenter Roeck 写道:
> On 12/21/21 6:01 PM, Jackie Liu wrote:
> > From: Jackie Liu <liuyun01@xxxxxxxxxx>
> >
> > Use devm_hwmon_device_register_with_info() and remove hwmon_dev
> > from ccp_device struct as it is not needed anymore.
> >
> > Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
> > ---
> >   drivers/hwmon/corsair-cpro.c | 15 ++++++---------
> >   1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> > index fa6aa4fc8b52..f476367ba6cf 100644
> > --- a/drivers/hwmon/corsair-cpro.c
> > +++ b/drivers/hwmon/corsair-cpro.c
> > @@ -76,7 +76,6 @@
> >   struct ccp_device {
> >       struct hid_device *hdev;
> > -    struct device *hwmon_dev;
> >       struct completion wait_input_report;
> >       struct mutex mutex; /* whenever buffer is used, lock before 
> > send_usb_cmd */
> >       u8 *buffer;
> > @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp)
> >   static int ccp_probe(struct hid_device *hdev, const struct 
> > hid_device_id *id)
> >   {
> >       struct ccp_device *ccp;
> > +    struct device *hwmon_dev;
> >       int ret;
> >       ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
> > @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, 
> > const struct hid_device_id *id)
> >       ret = get_fan_cnct(ccp);
> >       if (ret)
> >           goto out_hw_close;
> > -    ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, 
> > "corsaircpro",
> > -                             ccp, &ccp_chip_info, 0);
> > -    if (IS_ERR(ccp->hwmon_dev)) {
> > -        ret = PTR_ERR(ccp->hwmon_dev);
> > +    hwmon_dev =
> > +        devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
> > +                             ccp, &ccp_chip_info, 0);
> > +    ret = PTR_ERR_OR_ZERO(hwmon_dev);
> > +    if (ret)
> >           goto out_hw_close;
> > -    }
> >       return 0;
> > @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, 
> > const struct hid_device_id *id)
> >   static void ccp_remove(struct hid_device *hdev)
> >   {
> > -    struct ccp_device *ccp = hid_get_drvdata(hdev);
> > -
> > -    hwmon_device_unregister(ccp->hwmon_dev);
> >       hid_hw_close(hdev);
> >       hid_hw_stop(hdev);
>
> The point is that the above two functions need to be called _after_ the 
> hwmon device
> was removed. This patch changes the order and removes the hwmon device 
> after the hid
> functions have been removed.
>
> If you think this is valid you'll need to explain in detail why removal 
> order
> does not matter. Otherwise this patch deserves a NACK.
>
> Guenter

Hi Guenter

After adjusting the order here, there will be a small window for sysfs 
to continue to provide services. However, because hid has been
disconnected, the read and write interfaces will not get the actual data 
and returned by timeout. IMO this is not a big issue, but it's okay to 
not change it.

--
BR, Jackie Liu