| Message ID | 20220905172001.69244-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/1] Input: icn8505 - Utilize acpi_get_subsystem_id() | expand |
Hi Andy, On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote: > Replace open coded variant of recently introduced acpi_get_subsystem_id(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/input/touchscreen/chipone_icn8505.c | 29 ++++++--------------- > 1 file changed, 8 insertions(+), 21 deletions(-) > > diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c > index f9ca5502ac8c..bb5e63b87c5d 100644 > --- a/drivers/input/touchscreen/chipone_icn8505.c > +++ b/drivers/input/touchscreen/chipone_icn8505.c > @@ -364,32 +364,19 @@ static irqreturn_t icn8505_irq(int irq, void *dev_id) > > static int icn8505_probe_acpi(struct icn8505_data *icn8505, struct device *dev) > { > - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > - const char *subsys = "unknown"; > - struct acpi_device *adev; > - union acpi_object *obj; > - acpi_status status; > - > - adev = ACPI_COMPANION(dev); > - if (!adev) > - return -ENODEV; > + const char *subsys; > > - status = acpi_evaluate_object(adev->handle, "_SUB", NULL, &buffer); > - if (ACPI_SUCCESS(status)) { > - obj = buffer.pointer; > - if (obj->type == ACPI_TYPE_STRING) > - subsys = obj->string.pointer; > - else > - dev_warn(dev, "Warning ACPI _SUB did not return a string\n"); > - } else { > - dev_warn(dev, "Warning ACPI _SUB failed: %#x\n", status); > - buffer.pointer = NULL; > - } > + subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); > + if (IS_ERR(subsys) && PTR_ERR(subsys) != -ENODATA) > + return PTR_ERR(subsys); > + > + if (IS_ERR(subsys) && PTR_ERR(subsys) == -ENODATA) > + subsys = kstrdup_const("unknown", GFP_KERNEL); Do we really need kstrdup_const() here? This makes me wonder if we need to also have error handling here, and if we going to tip some automated tools by not having it. Why can't we simply assign the constant here (and continue using kfree_const() below)? I think this is the case where PTR_ERR_OR_ZERO() might help avoid multiple IS_ERR/PTR_ERR: subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); error = PTR_ERR_OR_ZERO(subsys); if (error == -ENODATA) subsys = "unknown"; else if (error) return error; > > snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name), > "chipone/icn8505-%s.fw", subsys); > > - kfree(buffer.pointer); > + kfree_const(subsys); > return 0; > } > Thanks.
On Mon, Sep 05, 2022 at 12:35:42PM -0700, Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote: ... > > + subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); > > + if (IS_ERR(subsys) && PTR_ERR(subsys) != -ENODATA) > > + return PTR_ERR(subsys); > > + > > + if (IS_ERR(subsys) && PTR_ERR(subsys) == -ENODATA) > > + subsys = kstrdup_const("unknown", GFP_KERNEL); > > Do we really need kstrdup_const() here? This makes me wonder if we need > to also have error handling here, and if we going to tip some automated > tools by not having it. Why can't we simply assign the constant here > (and continue using kfree_const() below)? Which makes code inconsistent. But okay, no big deal. > I think this is the case where PTR_ERR_OR_ZERO() might help avoid > multiple IS_ERR/PTR_ERR: > > subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); > error = PTR_ERR_OR_ZERO(subsys); > if (error == -ENODATA) > subsys = "unknown"; > else if (error) > return error; Would it matter? The generated code will be the same in both cases, no? > > snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name), > > "chipone/icn8505-%s.fw", subsys); > > > > - kfree(buffer.pointer); > > + kfree_const(subsys); > > return 0;
On Tue, Sep 06, 2022 at 03:54:06PM +0300, Andy Shevchenko wrote: > On Mon, Sep 05, 2022 at 12:35:42PM -0700, Dmitry Torokhov wrote: > > On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote: > > ... > > > > + subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); > > > + if (IS_ERR(subsys) && PTR_ERR(subsys) != -ENODATA) > > > + return PTR_ERR(subsys); > > > + > > > + if (IS_ERR(subsys) && PTR_ERR(subsys) == -ENODATA) > > > + subsys = kstrdup_const("unknown", GFP_KERNEL); > > > > Do we really need kstrdup_const() here? This makes me wonder if we need > > to also have error handling here, and if we going to tip some automated > > tools by not having it. Why can't we simply assign the constant here > > (and continue using kfree_const() below)? > > Which makes code inconsistent. But okay, no big deal. To me the *_const() APIs are needed when the code does not really know if it deals with a const/read-only object or not. If we know for sure we are dealing with a const/read-only object, we can skip allocation and freeing, so I do not see any inconsistencies. > > > I think this is the case where PTR_ERR_OR_ZERO() might help avoid > > multiple IS_ERR/PTR_ERR: > > > > subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); > > error = PTR_ERR_OR_ZERO(subsys); > > if (error == -ENODATA) > > subsys = "unknown"; > > else if (error) > > return error; > > Would it matter? The generated code will be the same in both cases, no? No, in the end I think the optimizer will reduce both variants to the same thing. I do find mine a bit more compact and thus easier to read, but I will not insist. Thanks.
diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c index f9ca5502ac8c..bb5e63b87c5d 100644 --- a/drivers/input/touchscreen/chipone_icn8505.c +++ b/drivers/input/touchscreen/chipone_icn8505.c @@ -364,32 +364,19 @@ static irqreturn_t icn8505_irq(int irq, void *dev_id) static int icn8505_probe_acpi(struct icn8505_data *icn8505, struct device *dev) { - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; - const char *subsys = "unknown"; - struct acpi_device *adev; - union acpi_object *obj; - acpi_status status; - - adev = ACPI_COMPANION(dev); - if (!adev) - return -ENODEV; + const char *subsys; - status = acpi_evaluate_object(adev->handle, "_SUB", NULL, &buffer); - if (ACPI_SUCCESS(status)) { - obj = buffer.pointer; - if (obj->type == ACPI_TYPE_STRING) - subsys = obj->string.pointer; - else - dev_warn(dev, "Warning ACPI _SUB did not return a string\n"); - } else { - dev_warn(dev, "Warning ACPI _SUB failed: %#x\n", status); - buffer.pointer = NULL; - } + subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev)); + if (IS_ERR(subsys) && PTR_ERR(subsys) != -ENODATA) + return PTR_ERR(subsys); + + if (IS_ERR(subsys) && PTR_ERR(subsys) == -ENODATA) + subsys = kstrdup_const("unknown", GFP_KERNEL); snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name), "chipone/icn8505-%s.fw", subsys); - kfree(buffer.pointer); + kfree_const(subsys); return 0; }
Replace open coded variant of recently introduced acpi_get_subsystem_id(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/input/touchscreen/chipone_icn8505.c | 29 ++++++--------------- 1 file changed, 8 insertions(+), 21 deletions(-)