| Message ID | 20241128154212.6216-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] platform/x86: int3472: Check for adev == NULL | expand |
On Thu, Nov 28, 2024 at 04:42:11PM +0100, Hans de Goede wrote: > The INT3472 code never wants a copy of the ACPI resource to be added > to the list-head passed to acpi_dev_get_resources(). > > Make skl_int3472_handle_gpio_resources() always return -errno or 1 > and drop the now no longer acpi_dev_free_resource_list() call. > > Also update the inaccurate comment about the return value. > skl_int3472_handle_gpio_resources() was already returning 1 in the case > of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV > as the comment claimed. ... > - acpi_dev_free_resource_list(&resource_list); Even though it's better to have this (no-op) call. As people may use the driver as an example and then make the real leakage somewhere else.
Hi, On 28-Nov-24 4:53 PM, Andy Shevchenko wrote: > On Thu, Nov 28, 2024 at 04:42:11PM +0100, Hans de Goede wrote: >> The INT3472 code never wants a copy of the ACPI resource to be added >> to the list-head passed to acpi_dev_get_resources(). >> >> Make skl_int3472_handle_gpio_resources() always return -errno or 1 >> and drop the now no longer acpi_dev_free_resource_list() call. >> >> Also update the inaccurate comment about the return value. >> skl_int3472_handle_gpio_resources() was already returning 1 in the case >> of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV >> as the comment claimed. > > ... > >> - acpi_dev_free_resource_list(&resource_list); > > Even though it's better to have this (no-op) call. As people may use the driver > as an example and then make the real leakage somewhere else. Ok, I'll keep the call for v2 and adjust the commit msg to match. Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 01da18b426ae..05e442078f8f 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -178,11 +178,11 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar * to create clocks and regulators via the usual frameworks. * * Return: - * * 1 - To continue the loop - * * 0 - When all resources found are handled properly. - * * -EINVAL - If the resource is not a GPIO IO resource - * * -ENODEV - If the resource has no corresponding _DSM entry - * * -Other - Errors propagated from one of the sub-functions. + * * 1 - Continue the loop without adding a copy of the resource to + * * the list passed to acpi_dev_get_resources() + * * 0 - Continue the loop after adding a copy of the resource to + * * the list passed to acpi_dev_get_resources() + * * -errno - Error, break loop */ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, void *data) @@ -283,7 +283,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, if (ret < 0) return dev_err_probe(int3472->dev, ret, err_msg); - return ret; + /* Tell acpi_dev_get_resources() to not make a copy of the resource */ + return 1; } static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) @@ -299,8 +300,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) if (ret < 0) return ret; - acpi_dev_free_resource_list(&resource_list); - /* Register _DSM based clock (no-op if a GPIO clock was already registered) */ ret = skl_int3472_register_dsm_clock(int3472); if (ret < 0)
The INT3472 code never wants a copy of the ACPI resource to be added to the list-head passed to acpi_dev_get_resources(). Make skl_int3472_handle_gpio_resources() always return -errno or 1 and drop the now no longer acpi_dev_free_resource_list() call. Also update the inaccurate comment about the return value. skl_int3472_handle_gpio_resources() was already returning 1 in the case of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV as the comment claimed. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Despite the "Fix" in the subject this is not really a bugfix, the old code works fine too, so no Fixes tag --- drivers/platform/x86/intel/int3472/discrete.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)