| 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: Wed, 5 Jun 2019 00:30:34 +0200
From: Pali Rohár <pali.rohar@...il.com>
To: Jean Delvare <jdelvare@...e.de>
Cc: Wolfram Sang <wsa@...-dreams.de>,
Michał Kępień <kernel@...pniu.pl>,
Steven Honeyman <stevenhoneyman@...il.com>,
Valdis.Kletnieks@...edu,
Jochen Eisinger <jochen@...guin-breeder.org>,
Gabriele Mazzotta <gabriele.mzt@...il.com>,
Andy Lutomirski <luto@...nel.org>, Mario_Limonciello@...l.com,
Alex Hung <alex.hung@...onical.com>,
Takashi Iwai <tiwai@...e.de>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3] i2c: i801: Register optional lis3lv02d i2c device on
Dell machines
Hi!
On Tuesday 04 June 2019 16:57:29 Jean Delvare wrote:
> Hi Pali,
>
> Next time, please start a new thread for a new version of a patch.
Ok!
> On Sun, 2 Jun 2019 15:20:03 +0200, Pali Rohár wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell Latitude
> > machines have ST microelectronics accelerometer at i2c address 0x29.
> >
> > Presence of that ST microelectronics accelerometer is verified by existence
> > of SMO88xx ACPI device which represent that accelerometer. Unfortunately
> > ACPI device does not specify i2c address.
> >
> > This patch registers lis3lv02d device for selected Dell Latitude machines
> > at i2c address 0x29 after detection. And for Dell Vostro V131 machine at
> > i2c address 0x1d which was manually detected.
> >
> > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to
> > conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so
> > lis3lv02d correctly initialize accelerometer.
> >
> > Tested on Dell Latitude E6440.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> >
> > ---
> > Changes since v2:
> > * Use explicit list of SMOxx ACPI devices
> >
> > Changes since v1:
> > * Added Dell Vostro V131 based on Michał Kępień testing
> > * Changed DMI product structure to include also i2c address
> > ---
> > drivers/i2c/busses/i2c-i801.c | 118 ++++++++++++++++++++++++++++++++++++
> > drivers/platform/x86/dell-smo8800.c | 1 +
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ac7f7817dc89..2ac8ff41cc24 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1134,6 +1134,121 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
> > }
> > }
> >
> > +/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
> > +static const struct acpi_device_id acpi_smo8800_ids[] = {
> > + { "SMO8800", 0 },
> > + { "SMO8801", 0 },
> > + { "SMO8810", 0 },
> > + { "SMO8811", 0 },
> > + { "SMO8820", 0 },
> > + { "SMO8821", 0 },
> > + { "SMO8830", 0 },
> > + { "SMO8831", 0 },
> > +};
> > +
> > +static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> > + u32 nesting_level,
> > + void *context,
> > + void **return_value)
> > +{
> > + struct acpi_device_info *info;
> > + acpi_status status;
> > + char *hid;
> > + int i;
> > +
> > + status = acpi_get_object_info(obj_handle, &info);
> > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > + return AE_OK;
> > +
> > + hid = info->hardware_id.string;
> > + if (!hid)
> > + return AE_OK;
> > +
> > + for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
> > + if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
> > + *((bool *)return_value) = true;
> > + return AE_CTRL_TERMINATE;
> > + }
> > + }
> > +
> > + return AE_OK;
> > +
>
> Unneeded blank line.
Ok.
> > +}
> > +
> > +static bool is_dell_system_with_lis3lv02d(void)
> > +{
> > + bool found;
> > + acpi_status status;
> > + const char *vendor;
> > +
> > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > + if (strcmp(vendor, "Dell Inc.") != 0)
> > + return false;
> > +
> > + /*
> > + * Check that ACPI device SMO88xx exists and is enabled. That ACPI
>
> I see how you check that the device exists, but not that it is enabled.
Hm.. you are right. I will add missing check.
> > + * device represent our ST microelectronics lis3lv02d accelerometer but
> > + * unfortunately without any other information (like i2c address).
>
> I2C
I will change i2c to I2C in whole patch.
> > + */
> > + found = false;
> > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > + (void **)&found);
> > + if (!ACPI_SUCCESS(status) || !found)
> > + return false;
> > +
> > + return true;
>
> Looks more complex than it needs to be. You don't really care about the
> status, as in the end you return the same on error as you do when no
> device is found. So I think you can simply go with:
>
> found = false;
> acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> (void **)&found);
>
> return found;
Ok, it really simplify that check.
>
> > +}
> > +
> > +/*
> > + * Accelerometer's i2c address is not specified in DMI nor ACPI,
>
> I2C
>
> > + * so it is needed to define mapping table based on DMI product names.
> > + */
> > +static struct {
>
> Any reason to not make it const?
No, I will make it const.
> > + const char *dmi_product_name;
> > + unsigned short i2c_addr;
> > +} dell_lis3lv02d_devices[] = {
> > + /*
> > + * Dell platform team told us that these Latitude devices have
> > + * ST microelectronics accelerometer at i2c address 0x29.
>
> I2C
>
> > + */
> > + { "Latitude E5250", 0x29 },
> > + { "Latitude E5450", 0x29 },
> > + { "Latitude E5550", 0x29 },
> > + { "Latitude E6440", 0x29 },
> > + { "Latitude E6440 ATG", 0x29 },
> > + { "Latitude E6540", 0x29 },
> > + /*
> > + * Additional individual entries were added after verification.
> > + */
> > + { "Vostro V131", 0x1d },
> > +};
> > +
> > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
> > +{
> > + struct i2c_board_info info;
> > + const char *dmi_product_name;
> > + int i;
> > +
> > + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
> > + if (strcmp(dmi_product_name,
> > + dell_lis3lv02d_devices[i].dmi_product_name) == 0)
> > + break;
> > + }
> > +
> > + if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
> > + dev_warn(&priv->pci_dev->dev,
> > + "Accelerometer lis3lv02d is present on i2c bus but its"
>
> i2c bus -> SMBus
>
> > + " i2c address is unknown, skipping registration...\n");
>
> I2C (or just s/i2c //, as it's kind of redundant)
>
> Suspension points not really needed IMHO.
Ok, it will be in V4 which I sent in few minutes.
> > + return;
> > + }
> > + (...)
>
> All the rest looks good to me.
>
> Thanks,
--
Pali Rohár
pali.rohar@...il.com
Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)
Powered by blists - more mailing lists