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
| ||
|
Date: Sat, 9 Jul 2022 16:46:00 +0200 From: Hans de Goede <hdegoede@...hat.com> To: Andy Shevchenko <andy.shevchenko@...il.com> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Mark Gross <markgross@...nel.org> Subject: Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection Hi Andy, On 7/9/22 13:34, Andy Shevchenko wrote: > On Sat, Jul 9, 2022 at 1:00 PM Hans de Goede <hdegoede@...hat.com> wrote: >> On 7/9/22 11:52, Andy Shevchenko wrote: >>> On Saturday, July 9, 2022, Hans de Goede <hdegoede@...hat.com <mailto:hdegoede@...hat.com>> wrote: >>> On 7/9/22 02:06, Andy Shevchenko wrote: > > ... > >>> So nack for this >>> >>> This effectively means nack to the series. > >>> But it’s easy to fix. I can add check for ret == 0. > > So, are you okay with fixing it this way? See below how. > >> I don't see how this is a nack for the series, just drop 1/7 + 2/7 >> and rebase the rest. Yes there will be conflicts to resolve in >> the rebase, but the rest of the cleanups can still go upstream >> after the rebase. > > Because patch 3 makes a little sense on its own if we drop the patch > 2. The rest is the simple cleanups which I do not consider as a core > of this series. Patch 3 just removes the adev == NULL check and pushes the ACPI_COMPANION(dev) calls into the function needing the adev, I don't see how that relies on this patch ? >>> > case SMI_AUTO_DETECT: >>> > - if (i2c_acpi_client_count(adev) > 0) >>> > - return smi_i2c_probe(pdev, adev, smi, node->instances); >>> > - else >>> > - return smi_spi_probe(pdev, adev, smi, node->instances); >>> > + ret = smi_i2c_probe(pdev, adev, smi, node->instances); >>> > + if (ret && ret != -ENOENT) >>> > + return ret; > > /* > * ...comment about why we do the following check... > */ > if (ret == 0) > return ret; I'm ok with doing things this way. Note you then end up with: if (ret && ret != -ENOENT) return ret; if (ret == 0) return ret; Which can be simplified to just: if (ret != -ENOENT) return ret; > >>> > + ret = smi_spi_probe(pdev, adev, smi, node->instances); >>> > + if (ret && ret != -ENOENT) >>> > + return ret; >>> > + if (ret) >>> > + return dev_err_probe(dev, ret, "Error No resources found\n"); >>> > + break; > > if (ret == -ENOENT) > return dev_err_probe(...); > return ret; Hmm, we don't do this dev_err for the SMI_I2C / SMI_SPI cases, I see 2 options to solve this: 1) Drop the return calls from switch (node->bus_type) {} instead always set ret and then break. And do the: if (ret == -ENOENT) return dev_err_probe(...); outside the switch-case 2) Drop the dev_err, the driver-core will already log an error for the -ENOENT anyways. Regards, Hans
Powered by blists - more mailing lists