linux-kernel - Re: [PATCH v1 1/1] Input: icn8505 - Utilize acpi_get_subsystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxZPjsRJXJijZ/K3@google.com>
Date:   Mon, 5 Sep 2022 12:35:42 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH v1 1/1] Input: icn8505 - Utilize acpi_get_subsystem_id()

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@...ux.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.

-- 
Dmitry