On Fri, Oct 11, 2019 at 7:52 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:
Hi Andrey,
On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov andrew.smirnov@gmail.com wrote:
To simplify resource management in commit that follows as well as to save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch driver code to use devres to manage the life-cycle of FF private data.
Signed-off-by: Andrey Smirnov andrew.smirnov@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org
This patch doesn't seem to fix any error, is there a reason to send it to stable? (besides as a dependency of the rest of the series).
Dependency is the only reason for this patch, but it is a pretty big reason. Prior to patches 1/3 and 2/3 FF private data was both allocated and passed off to FF layer via ff->private = data in a span of a few lines of code within hidpp_ff_init()/g920_get_config(). After, said pair is effectively split into two different functions, both needing access to FF private data, but called quite far apart in hidpp_probe(). Alternatives to patch 1/3 would be to either make sure that every error path in hidpp_prob() after the call to g920_allocate() is aware of allocated FF data, which seems like a nightmare, or, to create a temporary FF data, fill it in g920_get_config() and memdup() it in hidpp_ff_init(). Is that (the latter) the path that you prefer to take?
drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++--------------- 1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 0179f7ed77e5..58eb928224e5 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff) struct hidpp_ff_private_data *data = ff->private;
kfree(data->effect_ids);Is there any reasons we can not also devm alloc data->effect_ids?
No, but I was trying to limit the scope of this patch.
/** Set private to NULL to prevent input_ff_destroy() from* freeing our devres allocated memoryOuch. There is something wrong here: input_ff_destroy() calls kfree(ff->private), when the data has not been allocated by input_ff_create(). This seems to lack a little bit of symmetry.
I agree, I think it's a wart in FF API design.
*/ff->private = NULL;}
static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index) @@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index) const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice); struct ff_device *ff; struct hidpp_report response;
struct hidpp_ff_private_data *data;
struct hidpp_ff_private_data *data = hidpp->private_data; int error, j, num_slots; u8 version;@@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index) return error; }
data = kzalloc(sizeof(*data), GFP_KERNEL);if (!data)return -ENOMEM; data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);if (!data->effect_ids) {kfree(data);
if (!data->effect_ids) return -ENOMEM;
}
data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue"); if (!data->wq) { kfree(data->effect_ids);
kfree(data); return -ENOMEM; }@@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index) return 0; }
-static int hidpp_ff_deinit(struct hid_device *hid) +static void hidpp_ff_deinit(struct hid_device *hid) {
struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);struct input_dev *dev = hidinput->input;struct hidpp_ff_private_data *data;if (!dev) {hid_err(hid, "Struct input_dev not found!\n");return -EINVAL;}
struct hidpp_device *hidpp = hid_get_drvdata(hid);struct hidpp_ff_private_data *data = hidpp->private_data; hid_info(hid, "Unloading HID++ force feedback.\n");
data = dev->ff->private;if (!data) {I am pretty sure we might need to keep a test on data not being null.
OK, sure. Could you be more explicit in your reasoning next time though? I am assuming this is because hid_hw_stop() might be called before?
hid_err(hid, "Private data not found!\n");return -EINVAL;} destroy_workqueue(data->wq); device_remove_file(&hid->dev, &dev_attr_range);return 0;}
This whole hunk seems unrelated to the devm change. Can you extract a patch that just stores hidpp_ff_private_data in hidpp->private_data and then cleans up hidpp_ff_deinit() before switching it to devm? (or maybe not, see below)
Well it appears you are against the idea of leveraging devres in this series, so discussing the fate of said hunk seems moot.
After a few more thoughts, I don't think this mixing of devm and non devm is a good idea: we could say that the hidpp_ff_private_data and its effect_ids are both children of the input_dev, not the hid_device. And we would expect the whole thing to simplify with devm, but it's not, because ff is not supposed to be used with devm.
I have a feeling the whole ff logic is wrong in term of where things should be cleaned up, but I can not come up with a good hint on where to start. For example, destroy_workqueue() is called in hidpp_ff_deinit() where it might be racy, and maybe we should call it in hidpp_ff_destroy()...
Yeah, it probably should be moved to hidpp_ff_destroy(). Out of scope for this series though, I'll deal with it in a separate submission.
Last, you should base this patch on top of the for-next branch, we recently merged a fix for the FF drivers in the HID subsystem: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-ne...
Sure will do.
Would it be too complex to drop this patch from the series and do a proper follow up cleanup series that might not need to go to stable?
No it's alright. I'll submit a v2 of this series with only two patches and send a follow up after.
Thanks, Andrey Smirnov