| Message ID | 20170320093224.18541-2-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Mon, Mar 20, 2017 at 10:32:17AM +0100, Michał Kępień wrote: > Simplify error handling in acpi_fujitsu_bl_add() by moving code > responsible for setting up the input device to a separate function. Modularization is nice, two minor nits/questions below: > > Signed-off-by: Michał Kępień <kernel@kempniu.pl> > --- > drivers/platform/x86/fujitsu-laptop.c | 64 +++++++++++++++++++++-------------- > 1 file changed, 38 insertions(+), 26 deletions(-) > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > index f3ccef3d5a1e..2b0dcf989e2a 100644 > --- a/drivers/platform/x86/fujitsu-laptop.c > +++ b/drivers/platform/x86/fujitsu-laptop.c > @@ -590,6 +590,41 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = { > > /* ACPI device for LCD brightness control */ > > +static int acpi_fujitsu_bl_input_setup(struct acpi_device *device) > +{ > + struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device); > + struct input_dev *input; > + int error; > + > + fujitsu_bl->input = input = input_allocate_device(); > + if (!input) > + return -ENOMEM; > + > + snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys), > + "%s/video/input0", acpi_device_hid(device)); > + > + input->name = acpi_device_name(device); > + input->phys = fujitsu_bl->phys; > + input->id.bustype = BUS_HOST; > + input->id.product = 0x06; > + input->dev.parent = &device->dev; > + input->evbit[0] = BIT(EV_KEY); > + set_bit(KEY_BRIGHTNESSUP, input->keybit); > + set_bit(KEY_BRIGHTNESSDOWN, input->keybit); > + set_bit(KEY_UNKNOWN, input->keybit); > + > + error = input_register_device(input); > + if (error) > + goto err_free_input_dev; > + > + return 0; > + > +err_free_input_dev: > + input_free_device(input); This leaves fujitsu_bl->input pointing at freed memory. Can this be assigned only after successful registration? If not, it would be cleaner to NULL it on failure. > + > + return error; This return path could be cleaned up a bit: error = input_register_device(input); if (error) input_free_device(input); return error; But, this driver uses this "error/return 0" pattern pretty consistently, whereas most of the kernel uses ret instead of error, and will return ret on success and failure, relying on it being 0 in the successful case. Over the whole driver, we'd save several lines with the conversion and be more consistent with the rest of the kernel. But, local consistency is important too. Jonathan, do you have a preference for this driver?
On Wed, Mar 29, 2017 at 12:54:15PM -0700, Darren Hart wrote: > On Mon, Mar 20, 2017 at 10:32:17AM +0100, Micha?? K??pie?? wrote: > > + > > + return error; > > This return path could be cleaned up a bit: > > error = input_register_device(input); > if (error) > input_free_device(input); > > return error; > > But, this driver uses this "error/return 0" pattern pretty consistently, whereas > most of the kernel uses ret instead of error, and will return ret on success and > failure, relying on it being 0 in the successful case. Over the whole driver, > we'd save several lines with the conversion and be more consistent with the rest > of the kernel. But, local consistency is important too. Jonathan, do you have a > preference for this driver? I have no strong preferences, except to say that clarity is important. As I eluded to a few minutes ago, I agree that there's scope to address error handling and there is a case to be made for bringing it into line with the rest of the kernel. I think this can be addressed in a separate patch series though. The present series under consideration doesn't make the situation any worse (it actually improves it in some places) and introduces worthwhile changes. As such I don't see that we gain anything by delaying it in order to address what is, at the end of the day, a separate concern. Regards jonathan
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index f3ccef3d5a1e..2b0dcf989e2a 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -590,6 +590,41 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = { /* ACPI device for LCD brightness control */ +static int acpi_fujitsu_bl_input_setup(struct acpi_device *device) +{ + struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device); + struct input_dev *input; + int error; + + fujitsu_bl->input = input = input_allocate_device(); + if (!input) + return -ENOMEM; + + snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys), + "%s/video/input0", acpi_device_hid(device)); + + input->name = acpi_device_name(device); + input->phys = fujitsu_bl->phys; + input->id.bustype = BUS_HOST; + input->id.product = 0x06; + input->dev.parent = &device->dev; + input->evbit[0] = BIT(EV_KEY); + set_bit(KEY_BRIGHTNESSUP, input->keybit); + set_bit(KEY_BRIGHTNESSDOWN, input->keybit); + set_bit(KEY_UNKNOWN, input->keybit); + + error = input_register_device(input); + if (error) + goto err_free_input_dev; + + return 0; + +err_free_input_dev: + input_free_device(input); + + return error; +} + static int fujitsu_backlight_register(void) { struct backlight_properties props = { @@ -612,7 +647,6 @@ static int fujitsu_backlight_register(void) static int acpi_fujitsu_bl_add(struct acpi_device *device) { int state = 0; - struct input_dev *input; int error; if (!device) @@ -623,28 +657,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS); device->driver_data = fujitsu_bl; - fujitsu_bl->input = input = input_allocate_device(); - if (!input) { - error = -ENOMEM; - goto err_stop; - } - - snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys), - "%s/video/input0", acpi_device_hid(device)); - - input->name = acpi_device_name(device); - input->phys = fujitsu_bl->phys; - input->id.bustype = BUS_HOST; - input->id.product = 0x06; - input->dev.parent = &device->dev; - input->evbit[0] = BIT(EV_KEY); - set_bit(KEY_BRIGHTNESSUP, input->keybit); - set_bit(KEY_BRIGHTNESSDOWN, input->keybit); - set_bit(KEY_UNKNOWN, input->keybit); - - error = input_register_device(input); + error = acpi_fujitsu_bl_input_setup(device); if (error) - goto err_free_input_dev; + goto err_stop; error = acpi_bus_update_power(fujitsu_bl->acpi_handle, &state); if (error) { @@ -695,10 +710,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) return 0; err_unregister_input_dev: - input_unregister_device(input); - input = NULL; -err_free_input_dev: - input_free_device(input); + input_unregister_device(fujitsu_bl->input); err_stop: return error; }
Simplify error handling in acpi_fujitsu_bl_add() by moving code responsible for setting up the input device to a separate function. Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 64 +++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 26 deletions(-)