Re: [PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources

[Li Zetao <lizetao1@huawei.com>]

Hi,

在 2024/9/5 20:53, Benjamin Tissoires 写道:
> On Sep 04 2024, Li Zetao wrote:
>> By adding a custom action to the device, it can bind the hid resource
>> to the hid_device life cycle. The framework automatically close and stop
>> the hid resources before hid_device is released, and the users do not
>> need to pay attention to the timing of hid resource release.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/hid.h    |  2 ++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 30de92d0bf0f..71143c0a4a02 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
>>   }
>>   EXPORT_SYMBOL_GPL(hid_hw_close);
>>   
>> +static void hid_hw_close_and_stop(void *data)
>> +{
>> +	struct hid_device *hdev = data;
>> +
>> +	hid_hw_close(hdev);
>> +	hid_hw_stop(hdev);
>> +}
>> +
>> +/**
>> + * devm_hid_hw_start_and_open - manage hid resources through custom action
>> + *
>> + * @hdev: hid device
>> + * @connect_mask: which outputs to connect, see HID_CONNECT_*
>> + *
>> + * Bind the hid resource to the hid_device life cycle and register
>> + * an action to release the hid resource. The users do not need to
>> + * pay attention to the release of hid.
> 
> The only problem I see here is that this API is not completely "safe",
> in the sense that a driver using it can not use manual kzalloc/kfree.
> It is obvious, but probably not so much while looking at the comments
> from Guenter where he asked you to also remove the .remove() callback.
> 
> So in the docs, we should probably state that if the .remove() is any
> different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be
> use with that function.I'll add some comments to illustrate a scenario like this.

> 
> Cheers,
> Benjamin
> 
>> + */
>> +
>> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
>> +{
>> +	int ret;
>> +
>> +	ret = hid_hw_start(hdev, connect_mask);
>> +	if (ret) {
>> +		hid_err(hdev, "hw start failed with %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = hid_hw_open(hdev);
>> +	if (ret) {
>> +		hid_err(hdev, "hw open failed with %d\n", ret);
>> +		hid_hw_stop(hdev);
>> +		return ret;
>> +	}
>> +
>> +	return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
>> +
>>   /**
>>    * hid_hw_request - send report request to device
>>    *
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 121d5b8bc867..0ce217aa5f62 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
>>   void hid_hw_stop(struct hid_device *hdev);
>>   int __must_check hid_hw_open(struct hid_device *hdev);
>>   void hid_hw_close(struct hid_device *hdev);
>> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
>> +					    unsigned int connect_mask);
>>   void hid_hw_request(struct hid_device *hdev,
>>   		    struct hid_report *report, enum hid_class_request reqtype);
>>   int __hid_hw_raw_request(struct hid_device *hdev,
>> -- 
>> 2.34.1
>>

Thanks,
Li Zetao.