Re: [PATCH] ASoC: soc-core: fix DMI handling
On Wed, 10 Mar 2021 13:39:27 -0600, Pierre-Louis Bossart wrote:
> When DMI information is not present, trying to assign the card long
> name results in the following warning.
>
> WARNING KERN tegra-audio-graph-card sound: ASoC: no DMI vendor name!
>
> The initial solution suggested was to test if the card device is an
> ACPI one. This causes a regression visible to userspace on all Intel
> platforms, with UCM unable to load card profiles based on DMI
> information: the card devices are not necessarily ACPI ones, e.g. when
> the parent creates platform devices on Intel devices.
>
> To fix this problem, this patch exports the existing dmi_available
> variable and tests it in the ASoC core.
>
> Fixes: c014170408bc ("ASoC: soc-core: Prevent warning if no DMI table is
> present")
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/firmware/dmi_scan.c | 1 +
> sound/soc/soc-core.c| 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index d51ca0428bb8..f191a1f901ac 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -166,6 +166,7 @@ static int __init dmi_checksum(const u8 *buf, u8 len)
> static const char *dmi_ident[DMI_STRING_MAX];
> static LIST_HEAD(dmi_devices);
> int dmi_available;
> +EXPORT_SYMBOL_GPL(dmi_available);
>
> /*
> * Save a DMI string
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 16ba54eb8164..c7e4600b2dd4 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1574,7 +1574,7 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card,
> const char *flavour)
> if (card->long_name)
> return 0; /* long name already set by driver or from DMI */
>
> - if (!is_acpi_device_node(card->dev->fwnode))
> + if (!dmi_available)
> return 0;
>
> /* make up dmi long name as: vendor-product-version-board */
Fine with me.
Acked-by: Jean Delvare
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v1 1/7] PCI: Introduce pci_bus_*() printing macros when device is not available
On Mon, 8 Mar 2021 14:20:14 +0200, Andy Shevchenko wrote: > In some cases PCI device structure is not available and we want to print > information based on the bus and devfn parameters. For this cases introduce > pci_bus_*() printing macros and replace in existing users. > > Signed-off-by: Andy Shevchenko > --- > drivers/pci/probe.c | 12 +++- > include/linux/pci.h | 9 + > 2 files changed, 12 insertions(+), 9 deletions(-) > (...) Nice. Reviewed-by: Jean Delvare (If you introduced a _debug flavor of that, it could probably be used in drivers/pci/hotplug/pciehp_hpc.c.) -- Jean Delvare SUSE L3 Support
Re: [PATCH v1 7/7] i2c: i801: convert to use common P2SB accessor
Hi Andy,
On Mon, 8 Mar 2021 14:20:20 +0200, Andy Shevchenko wrote:
> Since we have a common P2SB accessor in tree we may use it instead of
> open coded variants.
>
> Replace custom code by pci_p2sb_bar() call.
I like the idea. Just two things...
> Signed-off-by: Andy Shevchenko
> ---
> drivers/i2c/busses/Kconfig| 1 +
> drivers/i2c/busses/i2c-i801.c | 40 ---
> drivers/pci/pci-p2sb.c| 6 ++
> 3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf7546e3f..ffd3007f888c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -101,6 +101,7 @@ config I2C_HIX5HD2
> config I2C_I801
> tristate "Intel 82801 (ICH/PCH)"
> depends on PCI
> + select PCI_P2SB if X86
> select CHECK_SIGNATURE if X86 && DMI
> select I2C_SMBUS
> help
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 4acee6f9e5a3..23b43de9786a 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -90,6 +90,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -136,7 +137,6 @@
> #define TCOBASE 0x050
> #define TCOCTL 0x054
>
> -#define SBREG_BAR0x10
> #define SBREG_SMBCTRL0xc6000c
> #define SBREG_SMBCTRL_DNV0xcf000c
>
> @@ -1524,52 +1524,30 @@ static const struct itco_wdt_platform_data
> spt_tco_platform_data = {
> .version = 4,
> };
>
> -static DEFINE_SPINLOCK(p2sb_spinlock);
> -
> static struct platform_device *
> i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>struct resource *tco_res)
> {
> struct resource *res;
> unsigned int devfn;
> - u64 base64_addr;
> - u32 base_addr;
> - u8 hidden;
> + int ret;
>
> /*
>* We must access the NO_REBOOT bit over the Primary to Sideband
> - * bridge (P2SB). The BIOS prevents the P2SB device from being
> - * enumerated by the PCI subsystem, so we need to unhide/hide it
> - * to lookup the P2SB BAR.
> + * bridge (P2SB).
>*/
> - spin_lock(_spinlock);
>
> devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
>
> - /* Unhide the P2SB device, if it is hidden */
> - pci_bus_read_config_byte(pci_dev->bus, devfn, 0xe1, );
> - if (hidden)
> - pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
> -
> - pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, _addr);
> - base64_addr = base_addr & 0xfff0;
> -
> - pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4,
> _addr);
> - base64_addr |= (u64)base_addr << 32;
> -
> - /* Hide the P2SB device, if it was hidden before */
> - if (hidden)
> - pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
> - spin_unlock(_spinlock);
> -
> res = _res[1];
> + ret = pci_p2sb_bar(pci_dev, devfn, res);
> + if (ret)
> + return ERR_PTR(ret);
> +
> if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
> - res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL_DNV;
> + res->start += SBREG_SMBCTRL_DNV;
> else
> - res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
> -
> - res->end = res->start + 3;
> - res->flags = IORESOURCE_MEM;
> + res->start += SBREG_SMBCTRL;
I can't see why you no longer set res->end and res->flags here. I can
imagine that pci_p2sb_bar() may have set the flags for us, but not that
->end is still correct after you fixed up ->start. Am I missing
something?
>
> return platform_device_register_resndata(_dev->dev, "iTCO_wdt", -1,
> tco_res, 2, _tco_platform_data,
> diff --git a/drivers/pci/pci-p2sb.c b/drivers/pci/pci-p2sb.c
> index 68d7dad48cdb..7f6bc7d4482a 100644
> --- a/drivers/pci/pci-p2sb.c
> +++ b/drivers/pci/pci-p2sb.c
> @@ -22,6 +22,12 @@
>
> static const struct x86_cpu_id p2sb_cpu_ids[] = {
> X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, PCI_DEVFN(13, 0)),
> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_D, PCI_DEVFN(31, 1)),
> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, PCI_DEVFN(31, 1)),
> + X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE,PCI_DEVFN(31, 1)),
> + X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE_L, PCI_DEVFN(31, 1)),
> + X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE, PCI_DEVFN(31, 1)),
> + X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_L, PCI_DEVFN(31, 1)),
> {}
> };
>
Any reason why this is added in this patch instead of [3/7] (PCI: New
Primary to Sideband (P2SB) bridge support library)?
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: sis630: Fix typo issue
On Mon, 01 Mar 2021 21:07:52 +0800, zuoqil...@163.com wrote: > From: zuoqilin > > Change 'adress' to 'address'. > > Signed-off-by: zuoqilin > --- > drivers/i2c/busses/i2c-sis630.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c > index cfb8e04..87d5625 100644 > --- a/drivers/i2c/busses/i2c-sis630.c > +++ b/drivers/i2c/busses/i2c-sis630.c > @@ -97,7 +97,7 @@ > module_param(force, bool, 0); > MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!"); > > -/* SMBus base adress */ > +/* SMBus base address */ > static unsigned short smbus_base; > > /* supported chips */ Reviewed-by: Jean Delvare -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: sis630: fix spellint typo
Hi zuoqilin, On Mon, 01 Mar 2021 09:40:26 +0800, zuoqil...@163.com wrote: > From: zuoqilin > > change 'adress' to 'address' > > Signed-off-by: zuoqilin > --- > drivers/i2c/busses/i2c-sis630.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c > index cfb8e04..87d5625 100644 > --- a/drivers/i2c/busses/i2c-sis630.c > +++ b/drivers/i2c/busses/i2c-sis630.c > @@ -97,7 +97,7 @@ > module_param(force, bool, 0); > MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!"); > > -/* SMBus base adress */ > +/* SMBus base address */ > static unsigned short smbus_base; > > /* supported chips */ I pointed out 4 issues in your original patch, you fixed only one and resubmitted with 3 issues remaining. I give up. Patch rejected, we can live with a spelling mistake. -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c/busses: fix spellint typo
Hi zuoqilin, There's an obvious typo in the subject. Which is kind of ironical considering the point of your patch. Also, your patch is driver-specific, so "i2c/busses:" isn't an appropriate prefix. According to the standard practice for the i2c subsystem, the proper prefix for the subject would be: "i2c: sis630:". On Thu, 25 Feb 2021 19:53:38 +0800, zuoqil...@163.com wrote: > From: zuoqilin > > change 'adress' to 'address' Please start your sentences with a capital and end them with a dot. > > Signed-off-by: zuoqilin > --- > drivers/i2c/busses/i2c-sis630.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c > index cfb8e04..87d5625 100644 > --- a/drivers/i2c/busses/i2c-sis630.c > +++ b/drivers/i2c/busses/i2c-sis630.c > @@ -97,7 +97,7 @@ > module_param(force, bool, 0); > MODULE_PARM_DESC(force, "Forcibly enable the SIS630. DANGEROUS!"); > > -/* SMBus base adress */ > +/* SMBus base address */ > static unsigned short smbus_base; > > /* supported chips */ Other than that, the change looks OK, thanks. -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: remove h from printk format specifier
Hi Tom, Wolfram,
On Tue, 5 Jan 2021 11:09:42 +0100, Wolfram Sang wrote:
> On Tue, Dec 15, 2020 at 10:33:27AM -0800, t...@redhat.com wrote:
> > From: Tom Rix
> >
> > See Documentation/core-api/printk-formats.rst.
> > h should no longer be used in the format specifier for printk.
> >
> > Signed-off-by: Tom Rix
>
> Adding Jean to CC. Jean, I'd think %02x would be better, what do you
> think?
Agreed, 0x%02x would be better.
If this is done then you can add:
Reviewed-by: Jean Delvare
> > ---
> > drivers/i2c/i2c-smbus.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index d3d06e3b4f3b..5cd2cf84659e 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -396,7 +396,7 @@ void i2c_register_spd(struct i2c_adapter *adap)
> >
> > if (!IS_ERR(i2c_new_scanned_device(adap, , addr_list,
> > NULL))) {
> > dev_info(>dev,
> > -"Successfully instantiated SPD at 0x%hx\n",
> > +"Successfully instantiated SPD at 0x%x\n",
> > addr_list[0]);
> > dimm_count--;
> > }
--
Jean Delvare
SUSE L3 Support
Re: False positive from checkscript: git git://...
On Thu, 29 Oct 2020 07:55:25 -0700, Joe Perches wrote: > On Thu, 2020-10-29 at 14:32 +0100, Jean Delvare wrote: > > WARNING: Possible repeated word: 'git' > > #20: FILE: MAINTAINERS:5289: > > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git > > dmi-for-next > > > > Obviously that's going to happen a lot as this is actually the proper > > way to list a git tree in that file. Could you please add an exception > > for that case? > > Already done in -next Perfect, thank you very much. -- Jean Delvare SUSE L3 Support
False positive from checkscript: git git://...
Hi Andy, Joe, I have hit this false positive from checkscript: WARNING: Possible repeated word: 'git' #20: FILE: MAINTAINERS:5289: +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-next Obviously that's going to happen a lot as this is actually the proper way to list a git tree in that file. Could you please add an exception for that case? Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] firmware/dmi: Include product_sku info to modalias
Hi Kai-Chuan,
On Wed, 28 Oct 2020 11:50:15 +0800, Kai-Chuan Hsieh wrote:
> Some Dell platforms rely on modalias to customize configuration,
> the product sku can be more specific for the hardware.
>
> Add product_sku to modalias for better utilization.
>
> Signed-off-by: Kai-Chuan Hsieh
> ---
> drivers/firmware/dmi-id.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index 86d71b0212b1..4d5421d14a41 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> @@ -85,6 +85,7 @@ static ssize_t get_modalias(char *buffer, size_t
> buffer_size)
> { "svn", DMI_SYS_VENDOR },
> { "pn", DMI_PRODUCT_NAME },
> { "pvr", DMI_PRODUCT_VERSION },
> + { "sku", DMI_PRODUCT_SKU },
> { "rvn", DMI_BOARD_VENDOR },
> { "rn", DMI_BOARD_NAME },
> { "rvr", DMI_BOARD_VERSION },
Applied, thanks.
For your future submissions, I invite you to read
Documentation/process/submitting-patches.rst
Specifically, when submitting a new version of a patch, please:
* Replace [PATCH] with [PATCH v2] in the subject.
* Do not reply to the previous version of the patch, instead start a
new thread.
* Ideally, include a list of changes from previous version, between the
"---" marker and the diffstat.
Thanks,
--
Jean Delvare
SUSE L3 Support
[PATCH] MAINTAINERS: The DMI/SMBIOS tree has moved
I switched from quilt to git as requested by Stephen Rothwell. Update the link to the new place. Signed-off-by: Jean Delvare Cc: Stephen Rothwell --- MAINTAINERS |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-5.10-rc1.orig/MAINTAINERS 2020-10-25 23:14:11.0 +0100 +++ linux-5.10-rc1/MAINTAINERS 2020-10-27 15:57:01.310612464 +0100 @@ -5286,7 +5286,7 @@ F:drivers/hwmon/dme1737.c DMI/SMBIOS SUPPORT M: Jean Delvare S: Maintained -T: quilt http://jdelvare.nerim.net/devel/linux/jdelvare-dmi/ +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-next F: Documentation/ABI/testing/sysfs-firmware-dmi-tables F: drivers/firmware/dmi-id.c F: drivers/firmware/dmi_scan.c -- Jean Delvare SUSE L3 Support
Re: [PATCH] firmware/dmi: Include product_sku info to modalias
Hi Kai-Chuan, On Thu, 22 Oct 2020 21:11:32 +0800, Kai-Chuan Hsieh wrote: > There are multiple product skus share the same product name, like > clamshell and 2-in-1 for Latitude series. > Both of them have 3-axis accelerator, but rotation is only disable for > clamshell model. > Originally, it should be descriminated by chassis_type, but found that > chassis_type is not trustful. > https://github.com/systemd/systemd/pull/17084#issuecomment-706931881 > Therefore, I would like to propose a change to include the product_sku > for applying customized configuration easier. OK. On the principle I'm fine with the change. As far as the implementation details go, I'd rather stick to 3 letters maximum as we did for other fields, to keep the overall string as short as possible. This also has the nice effect that you wouldn't need to realign everything, which would make the patch more readable too. So please go with either "sku" (my preference) or "psk". Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] firmware/dmi: Include product_sku info to modalias
Hi Kai-Chuan, On Thu, 22 Oct 2020 14:40:47 +0800, kaichuan.hs...@canonical.com wrote: > From: Kai-Chuan Hsieh > > Some Dell platforms rely on modalias to customize configuration, > the product sku can be more specific for the hardware. > > Add product_sku to modalias for better utilization. Do you have an actual use case for this already, or is it a theoretical concern? -- Jean Delvare SUSE L3 Support
[GIT PULL] dmi update for v5.10
Hi Linus, Please pull dmi subsystem updates/fixes for Linux v5.10 from: git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-linus drivers/firmware/dmi_scan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- Alexander A. Klimov (1): Replace HTTP links with HTTPS ones: DMI/SMBIOS SUPPORT Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: i801: Register lis3lv02d I2C device on Dell Latitude 5480
Hi Wolfram,
On Wed, 05 Aug 2020 11:33:47 +0200, w...@kernel.org wrote:
> On Tue, Jun 16, 2020 at 07:41:30PM -0400, Jeffrey Lin wrote:
> > Value of /sys/devices/platform/lis3lv02d/position when
> > Horizontal: (36,-108,-1152)
> > Left elevated: (-432,-126,-1062)
> > Front elevated: (36,594,-936)
> > Upside down:(-126,-252,1098)
> >
> > Signed-off-by: Jeffrey Lin
>
> Jean?
>
> > ---
> > drivers/i2c/busses/i2c-i801.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index fea644921a76..d7c802e20ae6 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1268,6 +1268,7 @@ static const struct {
> > /*
> > * Additional individual entries were added after verification.
> > */
> > + { "Latitude 5480", 0x29 },
> > { "Vostro V131",0x1d },
> > };
> >
No actual code change and not something I can test, so pretty much any
kernel developer is as qualified as I am to deal with it. But yes, OK,
I'm listed as the maintainer for this driver, so I should have replied
earlier, sorry.
Reviewed-by: Jean Delvare
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v2 2/2] i2c: eg20t: use generic power management
Hi Bjorn, Vaibhav, On Fri, 07 Aug 2020 15:23:21 -0500, Bjorn Helgaas wrote: > Also, i801_suspend() looks suspicious because it writes SMBHSTCFG, but > I don't see anything corresponding in i801_resume(). You're right, it's buggy. Volker Rümelin's patch at: https://patchwork.ozlabs.org/project/linux-i2c/patch/a2fc5a6d-a3bf-eaf0-bb75-1521be346...@googlemail.com/ should fix it. I was supposed to review it but did not, shame on me. I'll do it today. -- Jean Delvare SUSE L3 Support
Re: VAIO EEPROM support in at24
On Wed, 5 Aug 2020 16:36:55 +0200, Jean Delvare wrote: > 1* Do we actually need to use a struct resource? With the current >requirements, that looks overkill to me. We really only need the >start and end offsets of the masked area (or start and length). Or >do you plan to ever support multiple masked ranges, and >resource.child would be used to daisy-chain these ranges? Personally >I would wait until the need exists. Dang, turns out that the need already exists. I just found that the eeprom driver masks *2* areas of the Sony VAIO EEPROMs. I should know because I'm the one who made that change but that was 13 years ago and my memory doesn't go that far back. I'll think of a way to support that. Still not a big fan of daisy-chained resource structs though. Maybe a generic post-processing callback function would do... I'll give that a try. -- Jean Delvare SUSE L3 Support
Re: VAIO EEPROM support in at24
On Wed, 5 Aug 2020 20:14:28 +0200, Bartosz Golaszewski wrote:
> On Wed, Aug 5, 2020 at 4:36 PM Jean Delvare wrote:
> > I finally found the time to give it a try. Here's what my (tested)
> > prototype looks like:
>
> Hi Jean,
>
> this looks good at first glance.
>
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > (...)
> > @@ -427,6 +450,15 @@ static int at24_read(void *priv, unsigne
> >
> > pm_runtime_put(dev);
> >
> > + if ((at24->flags & AT24_FLAG_MASKED_RANGE) &&
> > !capable(CAP_SYS_ADMIN)) {
>
> Maybe use unlikely() here? It's not necessarily a hotpath but at least
> it would be obvious it's a corner case.
Sure.
> > (...)
> > 1* Do we actually need to use a struct resource? With the current
> >requirements, that looks overkill to me. We really only need the
> >start and end offsets of the masked area (or start and length). Or
> >do you plan to ever support multiple masked ranges, and
> >resource.child would be used to daisy-chain these ranges? Personally
> >I would wait until the need exists.
>
> Yes, since this change doesn't seem to commit to any stable ABI, I'd
> say we can drop the reference to struct resource and possibly add it
> in the future. This just was the first thing that came to mind when I
> suggested it.
OK, I changed it to simple integers for now.
> >Note that if we would just store mstart and mlen in struct
> >at24_chip_data then we could even get rid of AT24_FLAG_MASKED_RANGE,
> >as mlen > 0 would imply a masked range.
>
> Makes sense.
Done.
> > 2* I chose the name "eeprom-vaio" because "vaio" would be too generic.
> >I'm open to suggestions if you don't like that name.
>
> Are you sure there won't be any different models of vaio eeproms? How
> about '24c02-vaio' or 'eeprom-vaio-24c02'?
All I've seen were 24C02 but last time was a decade ago. I have no idea
if recent Vaio laptops still have this EEPROM, at this address, of that
size. 'eeprom-vaio-24c02' is too long to my taste, and kind of
redundant as '24c02' implies 'eeprom'. I like '24c02-vaio' very much
though, it is both concise and accurate, and is future-proof too. I'll
go for that, thanks for the suggestion.
> > 3* at24_read() was pretty elegant before my changes, but with the need
> >to remember the original value of many parameters, it no longer is.
> >I'm considering rewriting it in a way that does not modify the
> >parameters needed to process the masked range, either as part of
> >this patch or as a subsequent clean-up patch. That would hopefully
> >make the code elegant again.
>
> All clean-ups are welcome.
OK, I'll give it a try and see if I can tidy it up.
> > 4* I made the masking active only for non-root users as this is what
> >the legacy eeprom driver was doing. I hope that's OK with you.
> >
>
> Yes, it's fine with me. If more fine-grained control is needed we can
> probably extend it.
OK :-)
I have a patch almost ready, I'll submit v2 later today.
--
Jean Delvare
SUSE L3 Support
Re: VAIO EEPROM support in at24
Hi Bartosz,
On Tue, 17 Mar 2020 15:32:56 +0100, Bartosz Golaszewski wrote:
> wt., 17 mar 2020 o 15:14 Jean Delvare napisał(a):
> > Before I start implementing the idea above, I would like to know if
> > anyone objects to it, or has a better idea?
>
> Sounds good to me in general but isn't it something we could
> generalize a bit more?
>
> For instance we could make at24_chip_data struct look something like this:
>
> struct at24_chip_data {
> u32 byte_len;
> u8 flags;
> struct resource masked;
> };
>
> And we could introduce a new macro called AT24_CHIP_DATA_MASKED that
> would automacially set the AT24_FLAG_MASKED_RANGE flag and take
> another argument that would contain the address and size of the masked
> register range (we'd put it into the "masked" resource)?
>
> Other ideas are welcome too. I just think that making it
> SONY_VAIO-specific may be a bit limiting in the future.
I finally found the time to give it a try. Here's what my (tested)
prototype looks like:
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -8,9 +8,11 @@
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -38,6 +40,8 @@
#define AT24_FLAG_MAC BIT(2)
/* Does not auto-rollover reads to the next slave address. */
#define AT24_FLAG_NO_RDROL BIT(1)
+/* Contains an area that should not be exposed to non-root users */
+#define AT24_FLAG_MASKED_RANGE BIT(0)
/*
* I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
@@ -87,6 +91,7 @@ struct at24_data {
u16 page_size;
u8 flags;
+ struct resource masked;
struct nvmem_device *nvmem;
struct regulator *vcc_reg;
@@ -121,6 +126,7 @@ MODULE_PARM_DESC(at24_write_timeout, "Ti
struct at24_chip_data {
u32 byte_len;
u8 flags;
+ struct resource masked;
};
#define AT24_CHIP_DATA(_name, _len, _flags)\
@@ -128,6 +134,16 @@ struct at24_chip_data {
.byte_len = _len, .flags = _flags, \
}
+#define AT24_CHIP_DATA_MASKED(_name, _len, _flags, _mstart, _mlen) \
+ static const struct at24_chip_data _name = {\
+ .byte_len = _len, \
+ .flags = _flags | AT24_FLAG_MASKED_RANGE, \
+ .masked = { \
+ .start = _mstart, \
+ .end = _mstart + _mlen - 1, \
+ }, \
+ }
+
/* needs 8 addresses as A0-A2 are ignored */
AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
/* old variants can't be handled with this generic entry! */
@@ -144,6 +160,9 @@ AT24_CHIP_DATA(at24_data_24mac602, 64 /
/* spd is a 24c02 in memory DIMMs */
AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
+/* vaio is a 24c02 on some Sony laptops */
+AT24_CHIP_DATA_MASKED(at24_data_vaio, 2048 / 8,
+ AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0x00, 14);
AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
AT24_CHIP_DATA(at24_data_24cs04, 16,
AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
@@ -177,6 +196,7 @@ static const struct i2c_device_id at24_i
{ "24mac402", (kernel_ulong_t)_data_24mac402 },
{ "24mac602", (kernel_ulong_t)_data_24mac602 },
{ "spd",(kernel_ulong_t)_data_spd },
+ { "eeprom-vaio",(kernel_ulong_t)_data_vaio },
{ "24c04", (kernel_ulong_t)_data_24c04 },
{ "24cs04", (kernel_ulong_t)_data_24cs04 },
{ "24c08", (kernel_ulong_t)_data_24c08 },
@@ -389,6 +409,9 @@ static int at24_read(void *priv, unsigne
struct device *dev;
char *buf = val;
int ret;
+ unsigned int orig_off = off;
+ char *orig_buf = buf;
+ size_t orig_count = count;
at24 = priv;
dev = at24_base_client_dev(at24);
@@ -427,6 +450,15 @@ static int at24_read(void *priv, unsigne
pm_runtime_put(dev);
+ if ((at24->flags & AT24_FLAG_MASKED_RANGE) && !capable(CAP_SYS_ADMIN)) {
+ int i;
+
+ for (i = orig_off; i < orig_off + orig_count; i++) {
+ if (i >= at24->masked.start && i <= at24->masked.end)
+ orig_buf[i] = 0x00;
+ }
+ }
+
return 0;
}
@@ -654,6 +686,7 @@ static int at24_probe(struct i2c_client
at24->byte_len = byte_len;
at24->page_size = page_size;
at24->flags = flags;
+ at24->masked = cdata->masked;
at24->num_addresses =
Re: VAIO EEPROM support in at24
Hi Wolfram, Sorry, somehow this message of yours slipped through the cracks. On Tue, 17 Mar 2020 16:01:42 +0100, Wolfram Sang wrote: > > And we could introduce a new macro called AT24_CHIP_DATA_MASKED that > > would automacially set the AT24_FLAG_MASKED_RANGE flag and take > > another argument that would contain the address and size of the masked > > register range (we'd put it into the "masked" resource)? > > I am all for generic solutions. One thing to consider here is that we > need a generic way to detect the various types. I guess it will > always(?) be decided on some memory locations having specific values? In the case of Sony VAIO EEPROMs, they can be identified by the combination of the EEPROM's I2C address (always 0x57) and the value of the 4 bytes at register address 0x80 (would read either "PCG-" or "VGN-"). If that's not considered robust enough then I suppose we could improve it further by checking that the DMI vendor is "Sony Corporation". That being said, automatic detection was not even on my mind originally. If we had a specific type defined for these EEPROMs, as we do with SPD EEPROMs, then one could easily instantiate them from user-space using the "new_device" sysfs attribute at the I2C bus level. This is exactly how we have been doing it for SPD EEPROMs until recently, as you have just merged my patch set to automate this recently. And even then, it's still limited to x86 and specific systems at the moment. Incidentally, instantiating these Sony VAIO EEPROMs automatically would share some code with that patch set, so that might be a good sign that it's the right time to look into that. I may give a try to Bartosz's idea to make it somewhat generic if everybody agrees that's the way to go. I'm not deeply familiar with the at24 driver so I'm not sure how to do it, but hopefully it will get clearer as I progress. -- Jean Delvare SUSE L3 Support
Re: [PATCH for v5.9] i2c: Replace HTTP links with HTTPS ones
Hi Alexander, On Sun, 19 Jul 2020 21:35:53 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > (...) > Documentation/i2c/busses/i2c-ali1535.rst | 2 +- > Documentation/i2c/busses/i2c-ali15x3.rst | 2 +- > Documentation/i2c/busses/i2c-piix4.rst | 4 ++-- > drivers/i2c/busses/i2c-ali1535.c | 2 +- > drivers/i2c/busses/i2c-ali15x3.c | 2 +- > 5 files changed, 6 insertions(+), 6 deletions(-) The diffstat above does not match the changes below (specifically i2c-piix4.rst is NOT modified by your actual patch). > diff --git a/Documentation/i2c/busses/i2c-ali1535.rst > b/Documentation/i2c/busses/i2c-ali1535.rst > index 6941064730dc..3fe2bad63597 100644 > --- a/Documentation/i2c/busses/i2c-ali1535.rst > +++ b/Documentation/i2c/busses/i2c-ali1535.rst > @@ -28,7 +28,7 @@ Additionally, the sequencing of the SMBus transactions has > been modified to > be more consistent with the sequencing recommended by the manufacturer and > observed through testing. These changes are reflected in this driver and > can be identified by comparing this driver to the i2c-ali15x3 driver. For > -an overview of these chips see http://www.acerlabs.com > +an overview of these chips see https://www.acerlabs.com > (...) A quick visit to this website shows that it is dead and useless. The closest thing nowadays would be https://www.ali.com.tw/ however as far as I know ALI sold their x86 chipset business to Nvidia in 2006. I couldn't find information about these old chipsets on either website though, so I believe that the best course of action would be to strip the links and surrounding sentences. I understand this is beyond the scope of your current project. Do you want me to take care of that? -- Jean Delvare SUSE L3 Support
Re: [PATCH][next] i2c: busses: Use fallthrough pseudo-keyword
Hi Gustavo,
On Thu, 16 Jul 2020 17:03:07 -0500, Gustavo A. R. Silva wrote:
> Replace the existing /* fall through */ comments and its variants with
> the new pseudo-keyword macro fallthrough[1].
>
> [1]
> https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/i2c/busses/i2c-amd8111.c | 2 +-
> drivers/i2c/busses/i2c-i801.c| 8
> drivers/i2c/busses/i2c-viapro.c | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-amd8111.c
> b/drivers/i2c/busses/i2c-amd8111.c
> index 2b14fef5bf26..34862ad3423e 100644
> --- a/drivers/i2c/busses/i2c-amd8111.c
> +++ b/drivers/i2c/busses/i2c-amd8111.c
> @@ -381,7 +381,7 @@ static s32 amd8111_access(struct i2c_adapter * adap, u16
> addr,
> if (status)
> return status;
> len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
> - /* fall through */
> + fallthrough;
> case I2C_SMBUS_I2C_BLOCK_DATA:
> for (i = 0; i < len; i++) {
> status = amd_ec_read(smbus, AMD_SMB_DATA + i,
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 1fc7ae77753d..638e7f7c66cc 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1765,19 +1765,19 @@ static int i801_probe(struct pci_dev *dev, const
> struct pci_device_id *id)
> case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS1:
> case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS2:
> priv->features |= FEATURE_IDF;
> - /* fall through */
> + fallthrough;
> default:
> priv->features |= FEATURE_BLOCK_PROC;
> priv->features |= FEATURE_I2C_BLOCK_READ;
> priv->features |= FEATURE_IRQ;
> - /* fall through */
> + fallthrough;
> case PCI_DEVICE_ID_INTEL_82801DB_3:
> priv->features |= FEATURE_SMBUS_PEC;
> priv->features |= FEATURE_BLOCK_BUFFER;
> - /* fall through */
> + fallthrough;
> case PCI_DEVICE_ID_INTEL_82801CA_3:
> priv->features |= FEATURE_HOST_NOTIFY;
> - /* fall through */
> + fallthrough;
> case PCI_DEVICE_ID_INTEL_82801BA_2:
> case PCI_DEVICE_ID_INTEL_82801AB_3:
> case PCI_DEVICE_ID_INTEL_82801AA_3:
> diff --git a/drivers/i2c/busses/i2c-viapro.c b/drivers/i2c/busses/i2c-viapro.c
> index 05aa92a3fbe0..970ccdcbb889 100644
> --- a/drivers/i2c/busses/i2c-viapro.c
> +++ b/drivers/i2c/busses/i2c-viapro.c
> @@ -228,7 +228,7 @@ static s32 vt596_access(struct i2c_adapter *adap, u16
> addr,
> goto exit_unsupported;
> if (read_write == I2C_SMBUS_READ)
> outb_p(data->block[0], SMBHSTDAT0);
> - /* Fall through */
> + fallthrough;
> case I2C_SMBUS_BLOCK_DATA:
> outb_p(command, SMBHSTCMD);
> if (read_write == I2C_SMBUS_WRITE) {
Looks good to me, thanks for doing that.
Reviewed-by: Jean Delvare
However I see many occurrences in other drivers within
drivers/i2c/busses, which are not covered by this patch:
drivers/i2c/busses/i2c-designware-pcidrv.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-synquacer.c
drivers/i2c/busses/i2c-ibm_iic.c
drivers/i2c/busses/i2c-aspeed.c
drivers/i2c/busses/scx200_acb.c
drivers/i2c/busses/i2c-digicolor.c
drivers/i2c/busses/i2c-opal.c
Are they covered by another patch of yours?
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [RFC PATCH 15/35] i2c/busses: Tidy Success/Failure checks
On Mon, 13 Jul 2020 14:22:27 +0200, Saheed O. Bolarinwa wrote:
> Signed-off-by: "Saheed O. Bolarinwa"
> ---
> This patch depends on PATCH 15/35
Not possible, as this *is* patch 15/35. Not really worth mentioning
anyway, as it is expected that patches in a given series may depend on
any earlier patch in the same series.
>
> drivers/i2c/busses/i2c-ali15x3.c | 5 ++---
> drivers/i2c/busses/i2c-nforce2.c | 3 +--
> drivers/i2c/busses/i2c-sis5595.c | 15 +--
> 3 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ali15x3.c
> b/drivers/i2c/busses/i2c-ali15x3.c
> index 359ee3e0864a..c9e779cc184e 100644
> --- a/drivers/i2c/busses/i2c-ali15x3.c
> +++ b/drivers/i2c/busses/i2c-ali15x3.c
> @@ -167,11 +167,10 @@ static int ali15x3_setup(struct pci_dev *ALI15X3_dev)
> if(force_addr) {
> dev_info(_dev->dev, "forcing ISA address 0x%04X\n",
> ali15x3_smba);
> - if (0 != pci_write_config_word(ALI15X3_dev,
> - SMBBA,
> + if (pci_write_config_word(ALI15X3_dev, SMBBA,
> ali15x3_smba))
> goto error;
You can't possibly leave the code with such a ugly alignment and run
away. The whole point of tidying patches it to have more readable code
in the end, right?
> - if (0 != pci_read_config_word(ALI15X3_dev,
> + if (pci_read_config_word(ALI15X3_dev,
> SMBBA, ))
> goto error;
> if ((a & ~(ALI15X3_SMB_IOSIZE - 1)) != ali15x3_smba) {
> diff --git a/drivers/i2c/busses/i2c-nforce2.c
> b/drivers/i2c/busses/i2c-nforce2.c
> index 385f4f446f36..54d2985b7aaf 100644
> --- a/drivers/i2c/busses/i2c-nforce2.c
> +++ b/drivers/i2c/busses/i2c-nforce2.c
> @@ -327,8 +327,7 @@ static int nforce2_probe_smb(struct pci_dev *dev, int
> bar, int alt_reg,
> /* Older incarnations of the device used non-standard BARs */
> u16 iobase;
>
> - if (pci_read_config_word(dev, alt_reg, )
> - != 0) {
> + if (pci_read_config_word(dev, alt_reg, )) {
> dev_err(>dev, "Error reading PCI config for %s\n",
> name);
> return -EIO;
> diff --git a/drivers/i2c/busses/i2c-sis5595.c
> b/drivers/i2c/busses/i2c-sis5595.c
> index fbe3ee31eae3..b016f48519d3 100644
> --- a/drivers/i2c/busses/i2c-sis5595.c
> +++ b/drivers/i2c/busses/i2c-sis5595.c
> @@ -175,11 +175,9 @@ static int sis5595_setup(struct pci_dev *SIS5595_dev)
>
> if (force_addr) {
> dev_info(_dev->dev, "forcing ISA address 0x%04X\n",
> sis5595_base);
> - if (pci_write_config_word(SIS5595_dev, ACPI_BASE, sis5595_base)
> - != 0)
> + if (pci_write_config_word(SIS5595_dev, ACPI_BASE, sis5595_base))
> goto error;
> - if (pci_read_config_word(SIS5595_dev, ACPI_BASE, )
> - != 0)
> + if (pci_read_config_word(SIS5595_dev, ACPI_BASE, ))
> goto error;
> if ((a & ~(SIS5595_EXTENT - 1)) != sis5595_base) {
> /* doesn't work for some chips! */
> @@ -188,16 +186,13 @@ static int sis5595_setup(struct pci_dev *SIS5595_dev)
> }
> }
>
> - if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, )
> - != 0)
> + if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, ))
> goto error;
> if ((val & 0x80) == 0) {
> dev_info(_dev->dev, "enabling ACPI\n");
> - if (pci_write_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, val
> | 0x80)
> - != 0)
> + if (pci_write_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, val
> | 0x80))
> goto error;
> - if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, )
> - != 0)
> + if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, ))
> goto error;
> if ((val & 0x80) == 0) {
> /* doesn't work for some chips? */
Overall I'd be happy to have a more consistent style for checking
errors on PCI config registers access, so this seems to be going into
the right direction.
--
Jean Delvare
SUSE L3 Support
Re: [RFC PATCH 14/35] i2c/busses: Change PCIBIOS_SUCCESSFUL to 0
Hi Saheed,
On Mon, 13 Jul 2020 14:22:26 +0200, Saheed O. Bolarinwa wrote:
> In reference to the PCI spec (Chapter 2), PCIBIOS* is an x86 concept.
> Their scope should be limited within arch/x86.
Which PCI specification are you talking about here. In my "PCI Local
Bus Revision 2.3" specification (March 29, 2002), chapter 2 is about
Signal Definition and has nothing to do with the BIOS.
>
> Change all PCIBIOS_SUCCESSFUL to 0
>
> Signed-off-by: "Saheed O. Bolarinwa"
> ---
> drivers/i2c/busses/i2c-ali15x3.c | 4 ++--
> drivers/i2c/busses/i2c-nforce2.c | 2 +-
> drivers/i2c/busses/i2c-sis5595.c | 10 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
Hmmm. That seems to be a lot of changes to solve an essentially
theoretical problem (if a problem at all). I am not familiar enough
with the PCI subsystem to claim that it is fundamentally wrong, but
enough to say I'm skeptical.
PCI is a cross-architecture standard, and we can't possibly have the
return value of core functions such as pci_write_config_word follow
different conventions depending on the architecture, can we? Does
pci_write_config_word() currently return PCIBIOS_SUCCESSFUL on success
on x86 and 0 on success on other architectures? What about errors, do
we return positive, "PCIBIOS-specific" error codes on x86 and negative,
unix-like error codes on other architectures?
> diff --git a/drivers/i2c/busses/i2c-ali15x3.c
> b/drivers/i2c/busses/i2c-ali15x3.c
> index 02185a1cfa77..359ee3e0864a 100644
> --- a/drivers/i2c/busses/i2c-ali15x3.c
> +++ b/drivers/i2c/busses/i2c-ali15x3.c
> @@ -167,11 +167,11 @@ static int ali15x3_setup(struct pci_dev *ALI15X3_dev)
> if(force_addr) {
> dev_info(_dev->dev, "forcing ISA address 0x%04X\n",
> ali15x3_smba);
> - if (PCIBIOS_SUCCESSFUL != pci_write_config_word(ALI15X3_dev,
> + if (0 != pci_write_config_word(ALI15X3_dev,
> SMBBA,
> ali15x3_smba))
> goto error;
This leaves the code horribly aligned.
> - if (PCIBIOS_SUCCESSFUL != pci_read_config_word(ALI15X3_dev,
> + if (0 != pci_read_config_word(ALI15X3_dev,
> SMBBA, ))
> goto error;
> if ((a & ~(ALI15X3_SMB_IOSIZE - 1)) != ali15x3_smba) {
> diff --git a/drivers/i2c/busses/i2c-nforce2.c
> b/drivers/i2c/busses/i2c-nforce2.c
> index 777278386f58..385f4f446f36 100644
> --- a/drivers/i2c/busses/i2c-nforce2.c
> +++ b/drivers/i2c/busses/i2c-nforce2.c
> @@ -328,7 +328,7 @@ static int nforce2_probe_smb(struct pci_dev *dev, int
> bar, int alt_reg,
> u16 iobase;
>
> if (pci_read_config_word(dev, alt_reg, )
> - != PCIBIOS_SUCCESSFUL) {
> + != 0) {
> dev_err(>dev, "Error reading PCI config for %s\n",
> name);
> return -EIO;
> diff --git a/drivers/i2c/busses/i2c-sis5595.c
> b/drivers/i2c/busses/i2c-sis5595.c
> index c793a5c14cda..fbe3ee31eae3 100644
> --- a/drivers/i2c/busses/i2c-sis5595.c
> +++ b/drivers/i2c/busses/i2c-sis5595.c
> @@ -176,10 +176,10 @@ static int sis5595_setup(struct pci_dev *SIS5595_dev)
> if (force_addr) {
> dev_info(_dev->dev, "forcing ISA address 0x%04X\n",
> sis5595_base);
> if (pci_write_config_word(SIS5595_dev, ACPI_BASE, sis5595_base)
> - != PCIBIOS_SUCCESSFUL)
> + != 0)
> goto error;
> if (pci_read_config_word(SIS5595_dev, ACPI_BASE, )
> - != PCIBIOS_SUCCESSFUL)
> + != 0)
> goto error;
> if ((a & ~(SIS5595_EXTENT - 1)) != sis5595_base) {
> /* doesn't work for some chips! */
> @@ -189,15 +189,15 @@ static int sis5595_setup(struct pci_dev *SIS5595_dev)
> }
>
> if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, )
> - != PCIBIOS_SUCCESSFUL)
> + != 0)
> goto error;
> if ((val & 0x80) == 0) {
> dev_info(_dev->dev, "enabling ACPI\n");
> if (pci_write_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, val
> | 0x80)
> - != PCIBIOS_SUCCESSFUL)
> + != 0)
> goto error;
> if (pci_read_config_byte(SIS5595_dev, SIS5595_ENABLE_REG, )
> - != PCIBIOS_SUCCESSFUL)
> + != 0)
> goto error;
> if ((val & 0x80) == 0) {
> /* doesn't work for some chips? */
--
Jean Delvare
SUSE L3 Support
Re: linux-next: failing to fetch the dmi tree
Hi Stephen, On Wed, 15 Jul 2020 11:37:43 +0200, Jean Delvare wrote: > On Mon, 13 Jul 2020 09:11:02 +1000, Stephen Rothwell wrote: > > Jean, I don't suppose you would like to produce a git tree for me to > > fetch instead, as yours is the last quilt series I fetch (apart from > > Andrew's which is special). > > Actually, feel free to suppose. While a quilt tree fits my current > workflow better, I don't want to be the guy who makes your life more > difficult. Let me give it a try. OK, so my old for-next branch in quilt format is still at: http://jdelvare.nerim.net/devel/linux/jdelvare-dmi/ and what should be a git equivalent of it is at: git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-next If the latter works for you then I shall be ready to switch to it officially. If there's anything wrong, please let me know. Will you curse me if I ever rebase this branch? Thanks, -- Jean Delvare SUSE L3 Support
Re: linux-next: failing to fetch the dmi tree
Hi Stephen, On Mon, 13 Jul 2020 09:11:02 +1000, Stephen Rothwell wrote: > Attempting to fetch the dmi tree gets this error: > > http://jdelvare.nerim.net/devel/linux/jdelvare-dmi/series: > 2020-07-13 08:58:05 ERROR 403: Forbidden. > > I am still using the previously fetched quilt series. Sorry about that, there was a general failure on my provider's web server over last week-end, not something I could fix myself. It got eventually fixed on Monday morning (July 13th, CEST). > Jean, I don't suppose you would like to produce a git tree for me to > fetch instead, as yours is the last quilt series I fetch (apart from > Andrew's which is special). Actually, feel free to suppose. While a quilt tree fits my current workflow better, I don't want to be the guy who makes your life more difficult. Let me give it a try. -- Jean Delvare SUSE L3 Support
Re: [PATCH] Replace HTTP links with HTTPS ones: DMI/SMBIOS SUPPORT
On Wed, 08 Jul 2020 16:37:56 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov > --- > (...) > If you apply the patch, please let me know. > > > drivers/firmware/dmi_scan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > (...) Applied, thanks. -- Jean Delvare SUSE L3 Support
[PATCH 2/2] soc: ixp4xx: Really select helper drivers automatically
Kconfig claims that the ixp4xx-qmgr and ixp4xx-npe helper drivers
are selected automatically as needed. However this is not what the
Kconfig entries are doing. Convert depends to select to match the
help texts.
Signed-off-by: Jean Delvare
Cc: Krzysztof Halasa
---
drivers/crypto/Kconfig |4 +++-
drivers/net/ethernet/xscale/Kconfig |7 ---
drivers/net/wan/Kconfig |4 +++-
3 files changed, 10 insertions(+), 5 deletions(-)
--- linux-5.7.orig/drivers/net/ethernet/xscale/Kconfig 2020-06-01
01:49:15.0 +0200
+++ linux-5.7/drivers/net/ethernet/xscale/Kconfig 2020-06-23
13:05:23.071767146 +0200
@@ -6,8 +6,7 @@
config NET_VENDOR_XSCALE
bool "Intel XScale IXP devices"
default y
- depends on NET_VENDOR_INTEL && (ARM && ARCH_IXP4XX && \
- IXP4XX_NPE && IXP4XX_QMGR)
+ depends on NET_VENDOR_INTEL && (ARM && ARCH_IXP4XX)
---help---
If you have a network (Ethernet) card belonging to this class, say Y.
@@ -20,9 +19,11 @@ if NET_VENDOR_XSCALE
config IXP4XX_ETH
tristate "Intel IXP4xx Ethernet support"
- depends on ARM && ARCH_IXP4XX && IXP4XX_NPE && IXP4XX_QMGR
+ depends on ARM && ARCH_IXP4XX
select PHYLIB
select NET_PTP_CLASSIFY
+ select IXP4XX_NPE
+ select IXP4XX_QMGR
---help---
Say Y here if you want to use built-in Ethernet ports
on IXP4xx processor.
--- linux-5.7.orig/drivers/net/wan/Kconfig 2020-06-01 01:49:15.0
+0200
+++ linux-5.7/drivers/net/wan/Kconfig 2020-06-23 13:05:23.072767157 +0200
@@ -315,8 +315,10 @@ config DSCC4_PCI_RST
config IXP4XX_HSS
tristate "Intel IXP4xx HSS (synchronous serial port) support"
- depends on HDLC && IXP4XX_NPE && IXP4XX_QMGR
+ depends on HDLC
depends on ARCH_IXP4XX
+ select IXP4XX_NPE
+ select IXP4XX_QMGR
help
Say Y here if you want to use built-in HSS ports
on IXP4xx processor.
--- linux-5.7.orig/drivers/crypto/Kconfig 2020-06-01 01:49:15.0
+0200
+++ linux-5.7/drivers/crypto/Kconfig2020-06-25 00:04:11.570461001 +0200
@@ -308,11 +308,13 @@ config CRYPTO_DEV_TALITOS2
config CRYPTO_DEV_IXP4XX
tristate "Driver for IXP4xx crypto hardware acceleration"
- depends on ARCH_IXP4XX && IXP4XX_QMGR && IXP4XX_NPE
+ depends on ARCH_IXP4XX
select CRYPTO_LIB_DES
select CRYPTO_AEAD
select CRYPTO_AUTHENC
select CRYPTO_SKCIPHER
+ select IXP4XX_NPE
+ select IXP4XX_QMGR
help
Driver for the IXP4xx NPE crypto engine.
--
Jean Delvare
SUSE L3 Support
[PATCH 1/2] soc: ixp4xx: List the whole directory in MAINTAINERS
Mention the whole directory containing the ixp4xx soc drivers in MAINTAINERS instead of each driver separately. Otherwise changes done to Makefile and Kconfig will fail to find a matching entry. This will also let future drivers match without having to update the entry each time. Signed-off-by: Jean Delvare Cc: Krzysztof Halasa --- MAINTAINERS |6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) --- linux-5.7.orig/MAINTAINERS 2020-06-23 13:03:34.728650310 +0200 +++ linux-5.7/MAINTAINERS 2020-06-23 13:04:14.045055597 +0200 @@ -8654,10 +8654,8 @@ M: Krzysztof Halasa S: Maintained F: drivers/net/ethernet/xscale/ixp4xx_eth.c F: drivers/net/wan/ixp4xx_hss.c -F: drivers/soc/ixp4xx/ixp4xx-npe.c -F: drivers/soc/ixp4xx/ixp4xx-qmgr.c -F: include/linux/soc/ixp4xx/npe.h -F: include/linux/soc/ixp4xx/qmgr.h +F: drivers/soc/ixp4xx/ +F: include/linux/soc/ixp4xx/ INTEL IXP4XX RANDOM NUMBER GENERATOR SUPPORT M: Deepak Saxena -- Jean Delvare SUSE L3 Support
[GIT PULL] dmi update for v5.8
Hi Linus, Please pull dmi subsystem updates for Linux v5.8 from: git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-linus drivers/firmware/dmi-id.c | 6 ++ drivers/firmware/dmi_scan.c | 30 ++ include/linux/mod_devicetable.h | 2 ++ scripts/mod/file2alias.c| 2 ++ 4 files changed, 40 insertions(+) --- Erwan Velu (1): firmware/dmi: Report DMI Bios & EC firmware release Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] firmware/dmi: Report DMI Bios & EC firmware release
Hi Erwan, On Mon, 11 May 2020 19:10:52 +0200, Erwan Velu wrote: > Jean, I don't see my patches in the 5.7-rc series. > Is there anything wrong with them ? Nothing wrong with your patch, but unfortunately I missed the merge window and sent my pull request a few days too late, so it was not accepted. Your patch is still in my pending queue: http://jdelvare.nerim.net/devel/linux/jdelvare-dmi/ and therefore included in every linux-next snapshot, but it won't be merged in v5.7, you'll have to wait for v5.8. This is entirely my fault and I am sorry about that. -- Jean Delvare SUSE L3 Support
Re: [PATCH 3/3] firmware/dmi: Report DMI Embedded Firmware release
On Wed, 18 Sep 2019 11:43:21 +0200, Erwan Velu wrote:
> Servers that have a BMC encodes the release version of their firmware
> in the "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.
>
> This information is useful to know which release of the BMC is actually
> running.
> It could be used for some quirks, debugging sessions or inventory tasks.
>
> This patch extract these 2 fields in DMI_EMBEDDED_FW_MAJOR_RELEASE &
> DMI_EMBEDDED_FW_MINOR_RELEASE
>
> A typical output for a Dell system running the 3.75 bios is :
>
> [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_major
> 3
> [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_minor
> 75
> [root@t1700 ~]#
Same comment here as for previous patch, obviously.
Additionally, please run scripts/checkpatch.pl on your patch before you
resubmit, and address all the problems reported.
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 2/3] firmware/dmi: Report DMI Bios release
On Wed, 18 Sep 2019 11:43:20 +0200, Erwan Velu wrote:
> Some vendors like HPe or Dell, encodes the release version of their BIOS
encodes -> encode
> in the "System BIOS {Major|Minor} Release" fields of Type 0.
>
> This information is useful to know which release of the bios is actually
> running.
> It could be used for some quirks, debugging sessions or inventory tasks.
>
> This patch extract these 2 fields in DMI_BIOS_MAJOR_RELEASE &
> DMI_BIOS_MINOR_RELEASE.
>
> A typical output for a Dell system running the 65.27 bios is :
>
> [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_major
> 65
> [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_minor
> 27
> [root@t1700 ~]#
I don't think we want two fields. This adds quite some overhead, and
they are not independent from each other anyway. I'd rather have one
field with the values combined:
[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release
65.27
[root@t1700 ~]#
This would also be in line with how it was implemented in dmidecode. Is
there any reason to NOT go that route?
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields
Hi Erwan,
Sorry for the late answer.
On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote:
> In DMI type 0, there is several fields that encodes a release.
is -> are
encodes -> encode
> The dmi_save_release() function have the logic to check if the field is valid.
> If so, it reports the actual value.
>
> Signed-off-by: Erwan Velu
> ---
> drivers/firmware/dmi_scan.c | 26 ++
> 1 file changed, 26 insertions(+)
This patch introduces a warning:
drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not
used [-Wunused-function]
static void __init dmi_save_release(const struct dmi_header *dm, int slot,
^~~~
because you add a static function with no user. I understand that you
add a use later in the series, but it's not OK to introduce a warning
even if temporary. It also makes little sense to split the changes that
way as there is no way to cherry-pick one of the patches without the
rest. And it makes things more difficult to review too, as I can't
possibly judge if this function if right without also seeing where is
will be called and how.
So, please merge all the changes into a single patch.
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 35ed56b9c34f..202bd2c69d0f 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct
> dmi_header *dm, int slot,
> dmi_ident[slot] = p;
> }
>
> +static void __init dmi_save_release(const struct dmi_header *dm, int slot,
> + int index)
> +{
> + const u8 *d;
> + char *s;
> +
> + // If the table doesn't have the field, let's return
Please stick to C89-style comments (/* */) as used everywhere else in
this file.
> + if (dmi_ident[slot] || dm->length < index)
> + return;
> +
> + d = (u8 *) dm + index;
> +
> + // As per the specification,
> + // if the system doesn't have the field, the value is FF
> + if (d[0] == 0xFF)
> + return;
That's not exactly what the specification says. It says:
"If the system does not support the use of [the System BIOS Major
Release] field, the value is 0FFh for both this field and the System
BIOS Minor Release field." So unused is when both fields are 0xFF. You
can't test them independently.
Same goes for the Embedded Controller Firmware Release fields, even if
it is worded differently, the logic is the same.
> +
> + s = dmi_alloc(4);
> + if (!s)
> + return;
> +
> + sprintf(s, "%u", d[0]);
> +
> + dmi_ident[slot] = s;
> +}
> +
> static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
> int index)
> {
--
Jean Delvare
SUSE L3 Support
[GIT PULL] dmi fix for v5.4
Hi Linus, Please pull dmi subsystem fixes for Linux v5.4 from: git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-linus drivers/firmware/dmi_scan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- Jean Delvare (1): firmware: dmi: Fix unlikely out-of-bounds read in save_mem_devices Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH 4/4] i2c: i801: Instantiate SPD EEPROMs automatically
On Mon, 14 Oct 2019 18:22:15 +0800, kbuild test robot wrote:
> Hi Jean,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [cannot apply to v5.4-rc3 next-20191011]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see
> https://stackoverflow.com/a/37406982]
>
> url:
> https://github.com/0day-ci/linux/commits/Jean-Delvare/Instantiate-SPD-EEPROMs-at-boot-on-x86/20191014-174252
> config: i386-defconfig (attached as .config)
> compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot
>
> All errors (new ones prefixed by >>):
>
>drivers/i2c/busses/i2c-i801.c: In function 'i801_probe_optional_slaves':
> >> drivers/i2c/busses/i2c-i801.c:1319:11: error: 'struct i801_priv' has no
> >> member named 'mux_drvdata'
> if (!priv->mux_drvdata)
> ^~
>
> vim +1319 drivers/i2c/busses/i2c-i801.c
>
> 1295
> 1296/* Register optional slaves */
> 1297static void i801_probe_optional_slaves(struct i801_priv *priv)
> 1298{
> 1299/* Only register slaves on main SMBus channel */
> 1300if (priv->features & FEATURE_IDF)
> 1301return;
> 1302
> 1303if (apanel_addr) {
> 1304struct i2c_board_info info;
> 1305
> 1306memset(, 0, sizeof(struct i2c_board_info));
> 1307info.addr = apanel_addr;
> 1308strlcpy(info.type, "fujitsu_apanel",
> I2C_NAME_SIZE);
> 1309i2c_new_device(>adapter, );
> 1310}
> 1311
> 1312if (dmi_name_in_vendors("FUJITSU"))
> 1313dmi_walk(dmi_check_onboard_devices,
> >adapter);
> 1314
> 1315if (is_dell_system_with_lis3lv02d())
> 1316register_dell_lis3lv02d_i2c_device(priv);
> 1317
> 1318/* Instantiate SPD EEPROMs unless the SMBus is
> multiplexed */
> > 1319if (!priv->mux_drvdata)
> 1320i2c_register_spd(>adapter);
> 1321}
> 1322 #else
> 1323static void __init input_apanel_init(void) {}
> 1324static void i801_probe_optional_slaves(struct i801_priv *priv)
> {}
> 1325#endif /* CONFIG_X86 && CONFIG_DMI */
> 1326
Fixed, thanks.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 3/4] i2c: smbus: Add a way to instantiate SPD EEPROMs automatically
On Mon, 14 Oct 2019 18:21:50 +0800, kbuild test robot wrote:
> Hi Jean,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [cannot apply to v5.4-rc3 next-20191011]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see
> https://stackoverflow.com/a/37406982]
>
> url:
> https://github.com/0day-ci/linux/commits/Jean-Delvare/Instantiate-SPD-EEPROMs-at-boot-on-x86/20191014-174252
> config: sparc64-allmodconfig (attached as .config)
> compiler: sparc64-linux-gcc (GCC) 7.4.0
> reproduce:
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.4.0 make.cross ARCH=sparc64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot
>
> All warnings (new ones prefixed by >>):
>
>In file included from drivers/i2c/i2c-core-base.c:24:0:
>include/linux/i2c-smbus.h: In function 'i2c_register_spd':
> >> include/linux/i2c-smbus.h:52:9: warning: 'return' with a value, in
> >> function returning void
> return 0;
> ^
>include/linux/i2c-smbus.h:50:13: note: declared here
> static void i2c_register_spd(struct i2c_adapter *adap)
> ^~~~
>In file included from drivers/i2c/i2c-core-base.c:24:0:
>At top level:
>include/linux/i2c-smbus.h:50:13: warning: 'i2c_register_spd' defined but
> not used [-Wunused-function]
>
> vim +/return +52 include/linux/i2c-smbus.h
>
> 46
> 47#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> 48void i2c_register_spd(struct i2c_adapter *adap);
> 49#else
> 50static void i2c_register_spd(struct i2c_adapter *adap)
> 51{
> > 52return 0;
> 53}
> 54#endif
> 55
Fixed, thanks Elliot.
--
Jean Delvare
SUSE L3 Support
[PATCH 4/4] i2c: i801: Instantiate SPD EEPROMs automatically
Call the function to instantiate SPD EEPROMs automatically on the
main SMBus controller.
Multiplexed SMBus systems are excluded for now as they are more
complex to handle.
Signed-off-by: Jean Delvare
---
drivers/i2c/busses/i2c-i801.c |4
1 file changed, 4 insertions(+)
--- linux-5.3.orig/drivers/i2c/busses/i2c-i801.c2019-10-11
11:00:10.574348301 +0200
+++ linux-5.3/drivers/i2c/busses/i2c-i801.c 2019-10-11 11:20:03.741576063
+0200
@@ -1314,6 +1314,10 @@ static void i801_probe_optional_slaves(s
if (is_dell_system_with_lis3lv02d())
register_dell_lis3lv02d_i2c_device(priv);
+
+ /* Instantiate SPD EEPROMs unless the SMBus is multiplexed */
+ if (!priv->mux_drvdata)
+ i2c_register_spd(>adapter);
}
#else
static void __init input_apanel_init(void) {}
--
Jean Delvare
SUSE L3 Support
[PATCH 3/4] i2c: smbus: Add a way to instantiate SPD EEPROMs automatically
In simple cases we can instantiate SPD EEPROMs on the SMBus
automatically. Start with just DDR2, DDR3 and DDR4 on x86 for now,
and only for systems with no more than 4 memory slots. These
limitations may be lifted later.
Signed-off-by: Jean Delvare
---
drivers/i2c/i2c-smbus.c | 104 +-
include/linux/i2c-smbus.h | 11
2 files changed, 113 insertions(+), 2 deletions(-)
--- linux-5.3.orig/drivers/i2c/i2c-smbus.c 2019-10-04 15:04:16.601640711
+0200
+++ linux-5.3/drivers/i2c/i2c-smbus.c 2019-10-11 13:01:59.596425003 +0200
@@ -3,10 +3,11 @@
* i2c-smbus.c - SMBus extensions to the I2C protocol
*
* Copyright (C) 2008 David Brownell
- * Copyright (C) 2010 Jean Delvare
+ * Copyright (C) 2010-2019 Jean Delvare
*/
#include
+#include
#include
#include
#include
@@ -203,6 +204,107 @@ EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert
module_i2c_driver(smbalert_driver);
+/*
+ * SPD is not part of SMBus but we include it here for convenience as the
+ * target systems are the same.
+ * Restrictions to automatic SPD instantiation:
+ * - Only works if all filled slots have the same memory type
+ * - Only works for DDR2, DDR3 and DDR4 for now
+ * - Only works on systems with 1 to 4 memory slots
+ */
+#if IS_ENABLED(CONFIG_DMI)
+void i2c_register_spd(struct i2c_adapter *adap)
+{
+ int n, slot_count = 0, dimm_count = 0;
+ u16 handle;
+ u8 common_mem_type = 0x0, mem_type;
+ u64 mem_size;
+ const char *name;
+
+ while ((handle = dmi_memdev_handle(slot_count)) != 0x) {
+ slot_count++;
+
+ /* Skip empty slots */
+ mem_size = dmi_memdev_size(handle);
+ if (!mem_size)
+ continue;
+
+ /* Skip undefined memory type */
+ mem_type = dmi_memdev_type(handle);
+ if (mem_type <= 0x02) /* Invalid, Other, Unknown */
+ continue;
+
+ if (!common_mem_type) {
+ /* First filled slot */
+ common_mem_type = mem_type;
+ } else {
+ /* Check that all filled slots have the same type */
+ if (mem_type != common_mem_type) {
+ dev_warn(>dev,
+"Different memory types mixed, not
instantiating SPD\n");
+ return;
+ }
+ }
+ dimm_count++;
+ }
+
+ /* No useful DMI data, bail out */
+ if (!dimm_count)
+ return;
+
+ dev_info(>dev, "%d/%d memory slots populated (from DMI)\n",
+dimm_count, slot_count);
+
+ if (slot_count > 4) {
+ dev_warn(>dev,
+"Systems with more than 4 memory slots not supported
yet, not instantiating SPD\n");
+ return;
+ }
+
+ switch (common_mem_type) {
+ case 0x13: /* DDR2 */
+ case 0x18: /* DDR3 */
+ case 0x1C: /* LPDDR2 */
+ case 0x1D: /* LPDDR3 */
+ name = "spd";
+ break;
+ case 0x1A: /* DDR4 */
+ case 0x1E: /* LPDDR4 */
+ name = "ee1004";
+ break;
+ default:
+ dev_info(>dev,
+"Memory type 0x%02x not supported yet, not
instantiating SPD\n",
+common_mem_type);
+ return;
+ }
+
+ /*
+* We don't know in which slots the memory modules are. We could
+* try to guess from the slot names, but that would be rather complex
+* and unreliable, so better probe all possible addresses until we
+* have found all memory modules.
+*/
+ for (n = 0; n < slot_count && dimm_count; n++) {
+ struct i2c_board_info info;
+ unsigned short addr_list[2];
+
+ memset(, 0, sizeof(struct i2c_board_info));
+ strlcpy(info.type, name, I2C_NAME_SIZE);
+ addr_list[0] = 0x50 + n;
+ addr_list[1] = I2C_CLIENT_END;
+
+ if (i2c_new_probed_device(adap, , addr_list, NULL)) {
+ dev_info(>dev,
+"Successfully instantiated SPD at 0x%hx\n",
+addr_list[0]);
+ dimm_count--;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(i2c_register_spd);
+#endif
+
MODULE_AUTHOR("Jean Delvare ");
MODULE_DESCRIPTION("SMBus protocol extensions support");
MODULE_LICENSE("GPL");
--- linux-5.3.orig/include/linux/i2c-smbus.h2019-10-04 15:04:16.601640711
+0200
+++ linux-5.3/include/linux/i2c-smbus.h 2019-10-11 11:03:51.432166962 +0200
@@ -2,7 +2,7 @@
/*
*
[PATCH 2/4] firmware: dmi: Add dmi_memdev_handle
Add a utility function dmi_memdev_handle() which returns the DMI
handle associated with a given memory slot. This will allow kernel
drivers to iterate over the memory slots.
Signed-off-by: Jean Delvare
---
drivers/firmware/dmi_scan.c | 16
include/linux/dmi.h |2 ++
2 files changed, 18 insertions(+)
--- linux-5.3.orig/drivers/firmware/dmi_scan.c 2019-10-10 11:33:02.871034637
+0200
+++ linux-5.3/drivers/firmware/dmi_scan.c 2019-10-10 11:45:37.275549638
+0200
@@ -1151,3 +1151,19 @@ u8 dmi_memdev_type(u16 handle)
return 0x0; /* Not a valid value */
}
EXPORT_SYMBOL_GPL(dmi_memdev_type);
+
+/**
+ * dmi_memdev_handle - get the DMI handle of a memory slot
+ * @slot: slot number
+ *
+ * Return the DMI handle associated with a given memory slot, or %0x
+ * if there is no such slot.
+ */
+u16 dmi_memdev_handle(int slot)
+{
+ if (dmi_memdev && slot >= 0 && slot < dmi_memdev_nr)
+ return dmi_memdev[slot].handle;
+
+ return 0x; /* Not a valid value */
+}
+EXPORT_SYMBOL_GPL(dmi_memdev_handle);
--- linux-5.3.orig/include/linux/dmi.h 2019-10-10 11:33:02.871034637 +0200
+++ linux-5.3/include/linux/dmi.h 2019-10-10 11:34:46.146337207 +0200
@@ -114,6 +114,7 @@ extern bool dmi_match(enum dmi_field f,
extern void dmi_memdev_name(u16 handle, const char **bank, const char
**device);
extern u64 dmi_memdev_size(u16 handle);
extern u8 dmi_memdev_type(u16 handle);
+extern u16 dmi_memdev_handle(int slot);
#else
@@ -144,6 +145,7 @@ static inline void dmi_memdev_name(u16 h
const char **device) { }
static inline u64 dmi_memdev_size(u16 handle) { return ~0ul; }
static inline u8 dmi_memdev_type(u16 handle) { return 0x0; }
+static inline u16 dmi_memdev_handle(int slot) { return 0x; }
static inline const struct dmi_system_id *
dmi_first_match(const struct dmi_system_id *list) { return NULL; }
--
Jean Delvare
SUSE L3 Support
[PATCH 1/4] firmware: dmi: Remember the memory type
Store the memory type while walking the memory slots, and provide a
way to retrieve it later.
Signed-off-by: Jean Delvare
---
drivers/firmware/dmi_scan.c | 25 -
include/linux/dmi.h |2 ++
2 files changed, 26 insertions(+), 1 deletion(-)
--- linux-5.3.orig/drivers/firmware/dmi_scan.c 2019-10-08 14:27:23.783640227
+0200
+++ linux-5.3/drivers/firmware/dmi_scan.c 2019-10-08 16:35:35.442803880
+0200
@@ -35,6 +35,7 @@ static struct dmi_memdev_info {
const char *bank;
u64 size; /* bytes */
u16 handle;
+ u8 type;/* DDR2, DDR3, DDR4 etc */
} *dmi_memdev;
static int dmi_memdev_nr;
@@ -391,7 +392,7 @@ static void __init save_mem_devices(cons
u64 bytes;
u16 size;
- if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x12)
+ if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x13)
return;
if (nr >= dmi_memdev_nr) {
pr_warn(FW_BUG "Too many DIMM entries in SMBIOS table\n");
@@ -400,6 +401,7 @@ static void __init save_mem_devices(cons
dmi_memdev[nr].handle = get_unaligned(>handle);
dmi_memdev[nr].device = dmi_string(dm, d[0x10]);
dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);
+ dmi_memdev[nr].type = d[0x12];
size = get_unaligned((u16 *)[0xC]);
if (size == 0)
@@ -1128,3 +1130,24 @@ u64 dmi_memdev_size(u16 handle)
return ~0ull;
}
EXPORT_SYMBOL_GPL(dmi_memdev_size);
+
+/**
+ * dmi_memdev_type - get the memory type
+ * @handle: DMI structure handle
+ *
+ * Return the DMI memory type of the module in the slot associated with the
+ * given DMI handle, or 0x0 if no such DMI handle exists.
+ */
+u8 dmi_memdev_type(u16 handle)
+{
+ int n;
+
+ if (dmi_memdev) {
+ for (n = 0; n < dmi_memdev_nr; n++) {
+ if (handle == dmi_memdev[n].handle)
+ return dmi_memdev[n].type;
+ }
+ }
+ return 0x0; /* Not a valid value */
+}
+EXPORT_SYMBOL_GPL(dmi_memdev_type);
--- linux-5.3.orig/include/linux/dmi.h 2019-10-04 16:14:24.575714482 +0200
+++ linux-5.3/include/linux/dmi.h 2019-10-08 17:42:19.726907967 +0200
@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const
extern bool dmi_match(enum dmi_field f, const char *str);
extern void dmi_memdev_name(u16 handle, const char **bank, const char
**device);
extern u64 dmi_memdev_size(u16 handle);
+extern u8 dmi_memdev_type(u16 handle);
#else
@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_fi
static inline void dmi_memdev_name(u16 handle, const char **bank,
const char **device) { }
static inline u64 dmi_memdev_size(u16 handle) { return ~0ul; }
+static inline u8 dmi_memdev_type(u16 handle) { return 0x0; }
static inline const struct dmi_system_id *
dmi_first_match(const struct dmi_system_id *list) { return NULL; }
--
Jean Delvare
SUSE L3 Support
[PATCH 0/4] Instantiate SPD EEPROMs at boot on x86
This is my work to let decode-dimms work out of the box on most x86 desktop and laptop systems. [PATCH 1/4] firmware: dmi: Remember the memory type [PATCH 2/4] firmware: dmi: Add dmi_memdev_handle [PATCH 3/4] i2c: smbus: Add a way to instantiate SPD EEPROMs automatically [PATCH 4/4] i2c: i801: Instantiate SPD EEPROMs automatically -- Jean Delvare SUSE L3 Support
[PATCH] firmware: dmi: Fix unlikely out-of-bounds read in save_mem_devices
Before reading the Extended Size field, we should ensure it fits in
the DMI record. There is already a record length check but it does
not cover that field.
It would take a seriously corrupted DMI table to hit that bug, so no
need to worry, but we should still fix it.
Signed-off-by: Jean Delvare
Fixes: 6deae96b42eb ("firmware, DMI: Add function to look up a handle and
return DIMM size")
Cc: Tony Luck
Cc: Borislav Petkov
---
drivers/firmware/dmi_scan.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-5.3.orig/drivers/firmware/dmi_scan.c 2019-10-04 16:14:24.575714482
+0200
+++ linux-5.3/drivers/firmware/dmi_scan.c 2019-10-04 16:18:18.878669652
+0200
@@ -408,7 +408,7 @@ static void __init save_mem_devices(cons
bytes = ~0ull;
else if (size & 0x8000)
bytes = (u64)(size & 0x7fff) << 10;
- else if (size != 0x7fff)
+ else if (size != 0x7fff || dm->length < 0x20)
bytes = (u64)size << 20;
else
bytes = (u64)get_unaligned((u32 *)[0x1C]) << 20;
--
Jean Delvare
SUSE L3 Support
[PATCH] eeprom: Warn that the driver is deprecated
Deprecating the driver in Kconfig is one thing, but we also need to let the users themselves know. Log a warning each time a device is bound to the deprecated eeprom driver. Signed-off-by: Jean Delvare Cc: Arnd Bergmann Cc: Greg Kroah-Hartman --- drivers/misc/eeprom/eeprom.c |4 1 file changed, 4 insertions(+) --- linux-5.2.orig/drivers/misc/eeprom/eeprom.c 2019-07-08 00:41:56.0 +0200 +++ linux-5.2/drivers/misc/eeprom/eeprom.c 2019-10-02 10:46:23.363974060 +0200 @@ -175,6 +175,10 @@ static int eeprom_probe(struct i2c_clien } } + /* Let the users know they are using deprecated driver */ + dev_notice(>dev, + "eeprom driver is deprecated, please use at24 instead\n"); + /* create the sysfs eeprom file */ return sysfs_create_bin_file(>dev.kobj, _attr); } -- Jean Delvare SUSE L3 Support
Re: /dev/mem and secure boot
Hi Greg, On Fri, 6 Sep 2019 14:15:10 +0200, Greg Kroah-Hartman wrote: > On Fri, Sep 06, 2019 at 01:02:21PM +0200, Jean Delvare wrote: > > I've been bitten recently by mcelog not working on machines started in > > secure boot mode. mcelog tries to read DMI information from /dev/mem > > and fails to open it. > > What do you mean by "secure boot"? Is this matthew's patchset that > restricts /dev/mem/ or something else? I mean that early in the kernel log is: Secure boot enabled and kernel locked down Digging it up, I found that this comes from a patch in our SLES kernel, that's not upstream (yet). It is from a patch set by David Howells (Cc'd) posted in April 2017: https://patchwork.kernel.org/patch/9665591/ https://patchwork.kernel.org/patch/9665015/ I wrongly assumed it had been merged upstream meanwhile but I was wrong. David, any reason why this didn't happen? Out of curiosity, are these patches in RHEL kernels? > > This made me wonder: if not even root can read /dev/mem (nor, I > > suppose, /dev/kmem and /dev/port) in secure boot mode, why are we > > creating these device nodes at all in the first place? Can't we detect > > that we are in secure boot mode and skip that step, and reap the rewards > > (faster boot, lower memory footprint and less confusion)? > > Sure, feel free to not register it at all if the mode is enabled. Now I feel sorry that I asked my question upstream when there's nothing to be done there. I'll go bother SUSE kernel folks instead, sorry for the noise. And thanks for the advice. -- Jean Delvare SUSE L3 Support
Re: /dev/mem and secure boot
D'oh, I hit reply while still editing... Sorry for the noise, a better answer will come later when ready. -- Jean Delvare SUSE L3 Support
Re: /dev/mem and secure boot
On Fri, 6 Sep 2019 14:15:10 +0200, Greg Kroah-Hartman wrote: > On Fri, Sep 06, 2019 at 01:02:21PM +0200, Jean Delvare wrote: > > I've been bitten recently by mcelog not working on machines started in > > secure boot mode. mcelog tries to read DMI information from /dev/mem > > and fails to open it. > > What do you mean by "secure boot"? Is this matthew's patchset that > restricts /dev/mem/ or something else? I mean that early in the kernel log is: Secure boot enabled and kernel locked down > > This made me wonder: if not even root can read /dev/mem (nor, I > > suppose, /dev/kmem and /dev/port) in secure boot mode, why are we > > creating these device nodes at all in the first place? Can't we detect > > that we are in secure boot mode and skip that step, and reap the rewards > > (faster boot, lower memory footprint and less confusion)? > > Sure, feel free to not register it at all if the mode is enabled. > > thanks, > > greg k-h -- Jean Delvare SUSE L3 Support
Re: [PATCH] eeprom: Deprecate the legacy eeprom driver
Hi Greg, On Wed, 4 Sep 2019 09:57:29 +0200, Greg Kroah-Hartman wrote: > On Mon, Sep 02, 2019 at 10:48:38AM +0200, Jean Delvare wrote: > > Time has come to get rid of the old eeprom driver. The at24 driver > > should be used instead. So mark the eeprom driver as deprecated and > > give users some time to migrate. Then we can remove the legacy > > eeprom driver completely. > > > > Signed-off-by: Jean Delvare > > Cc: Arnd Bergmann > > --- > > drivers/misc/eeprom/Kconfig |5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > You might also want to add a big printk() message when the driver is > loaded that it shouldn't be used. Good idea, although unfortunately this means expanding module_i2c_driver. Or maybe I can use printk_once() in eeprom_probe(). Or even just a dev_warn() there to really spam the kernel log in a very visible way. Would you prefer a v2 of this patch including that change, or a separate, incremental patch? Thanks, -- Jean Delvare SUSE L3 Support
/dev/mem and secure boot
I've been bitten recently by mcelog not working on machines started in secure boot mode. mcelog tries to read DMI information from /dev/mem and fails to open it. This made me wonder: if not even root can read /dev/mem (nor, I suppose, /dev/kmem and /dev/port) in secure boot mode, why are we creating these device nodes at all in the first place? Can't we detect that we are in secure boot mode and skip that step, and reap the rewards (faster boot, lower memory footprint and less confusion)? Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt
i801_isr_byte_done(priv);
@@ -1006,9 +1038,35 @@ static void i801_enable_host_notify(stru
outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
}
-static void i801_disable_host_notify(struct i801_priv *priv)
+static void i801_enable_smbus_alert(struct i2c_adapter *adapter)
{
- if (!(priv->features & FEATURE_HOST_NOTIFY))
+ struct i801_priv *priv = i2c_get_adapdata(adapter);
+
+ if (!(priv->features & FEATURE_SMBUS_ALERT))
+ return;
+
+ priv->ara = i2c_setup_smbus_alert(adapter, >alert_data);
+ if (!priv->ara) {
+ dev_warn(>dev, "Failed to register ARA client\n");
+
+ /* Disable SMBus Alert interrupts */
+ if (!(SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd))
+ outb_p(SMBSLVCMD_SMBALERT_DIS | priv->original_slvcmd,
+ SMBSLVCMD(priv));
+ return;
+ }
+
+ if (SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd)
+ outb_p(~SMBSLVCMD_SMBALERT_DIS & priv->original_slvcmd,
+ SMBSLVCMD(priv));
+
+ /* Clear SMBus Alert bit to allow a new notification */
+ outb_p(SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
+}
+
+static void i801_restore_slvcmd(struct i801_priv *priv)
+{
+ if (!(priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT)))
return;
outb_p(priv->original_slvcmd, SMBSLVCMD(priv));
@@ -1823,8 +1881,8 @@ static int i801_probe(struct pci_dev *de
outb_p(inb_p(SMBAUXCTL(priv)) &
~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
- /* Remember original Host Notify setting */
- if (priv->features & FEATURE_HOST_NOTIFY)
+ /* Remember original Host Notify and SMBus Alert setting */
+ if (priv->features & (FEATURE_HOST_NOTIFY | FEATURE_SMBUS_ALERT))
priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
/* Default timeout in interrupt mode: 200 ms */
@@ -1875,6 +1933,7 @@ static int i801_probe(struct pci_dev *de
}
i801_enable_host_notify(>adapter);
+ i801_enable_smbus_alert(>adapter);
i801_probe_optional_slaves(priv);
/* We ignore errors - multiplexing is optional */
@@ -1897,8 +1956,10 @@ static void i801_remove(struct pci_dev *
pm_runtime_forbid(>dev);
pm_runtime_get_noresume(>dev);
- i801_disable_host_notify(priv);
+ i801_restore_slvcmd(priv);
i801_del_mux(priv);
+ if (priv->ara)
+ i2c_unregister_device(priv->ara);
i2c_del_adapter(>adapter);
i801_acpi_remove(priv);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
@@ -1916,7 +1977,7 @@ static void i801_shutdown(struct pci_dev
struct i801_priv *priv = pci_get_drvdata(dev);
/* Restore config registers to avoid hard hang on some systems */
- i801_disable_host_notify(priv);
+ i801_restore_slvcmd(priv);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
}
--
Jean Delvare
SUSE L3 Support
[PATCH] eeprom: Deprecate the legacy eeprom driver
Time has come to get rid of the old eeprom driver. The at24 driver should be used instead. So mark the eeprom driver as deprecated and give users some time to migrate. Then we can remove the legacy eeprom driver completely. Signed-off-by: Jean Delvare Cc: Arnd Bergmann Cc: Greg Kroah-Hartman --- drivers/misc/eeprom/Kconfig |5 - 1 file changed, 4 insertions(+), 1 deletion(-) --- linux-5.2.orig/drivers/misc/eeprom/Kconfig 2019-08-23 18:40:44.140314063 +0200 +++ linux-5.2/drivers/misc/eeprom/Kconfig 2019-09-02 10:44:05.633190675 +0200 @@ -45,13 +45,16 @@ config EEPROM_AT25 will be called at25. config EEPROM_LEGACY - tristate "Old I2C EEPROM reader" + tristate "Old I2C EEPROM reader (DEPRECATED)" depends on I2C && SYSFS help If you say yes here you get read-only access to the EEPROM data available on modern memory DIMMs and Sony Vaio laptops via I2C. Such EEPROMs could theoretically be available on other devices as well. + This driver is deprecated and will be removed soon, please use the + better at24 driver instead. + This driver can also be built as a module. If so, the module will be called eeprom. -- Jean Delvare SUSE L3 Support
Re: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT interrupt
Hi Lingyan,
On Mon, 12 Aug 2019 10:40:34 +0800, lingyxu wrote:
> From: Lingyan Xu
>
> In current i801 driver, SMBALERT interrupt is allowed
> (Slave Command Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr,
> if there is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
>
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
>
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
>
> Signed-off-by: Lingyan Xu
> ---
> drivers/i2c/busses/i2c-i801.c |7 ++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,11 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>* Clear irq sources and report transaction result.
>* ->status must be cleared before the next transaction is started.
>*/
> +
> + outb_p(status, SMBHSTSTS(priv));
> +
> status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
> if (status) {
> - outb_p(status, SMBHSTSTS(priv));
> priv->status = status;
> wake_up(>waitq);
> }
Looks scary. Writing the whole value of SMBHSTSTS back to itself
without selecting which bits you write is dangerous. Specifically,
writing back SMBHSTSTS_BYTE_DONE, SMBHSTSTS_INUSE_STS and
SMBHSTSTS_HOST_BUSY could have unexpected consequences. I would feel
much better if you would just explicitly add SMBHSTSTS_SMBALERT_STS to
the list.
> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> /* Default timeout in interrupt mode: 200 ms */
> priv->adapter.timeout = HZ / 5;
>
> + /* Disable SMBALERT interrupt */
> + outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));
Please give SMBSLVCMD's BIT(2) a name and define it after
SMBSLVCMD_HST_NTFY_INTREN.
Also it is mandatory to restore the value of SMBSLVCMD before returning
the control back to the BIOS. Currently this is only being done when
the FEATURE_HOST_NOTIFY bit is set because that's the only case where
we change the value of that register, but if we change it
unconditionally then it must be saved and restored unconditionally too.
> +
> if (dev->irq == IRQ_NOTCONNECTED)
> priv->features &= ~FEATURE_IRQ;
>
That being said, if you see this interrupt flood, it means that at
least one device on your SMBus would benefit from SMBus Alert being
supported. The infrastructure is already there as we added support in a
few I2C bus drivers already. So maybe instead of silencing the
interrupts, we could add proper SMBus Alert support to the i2c-i801
driver?
Did you figure out which device is raising the SMBus Alert and why?
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v5 0/3] Enable ACPI-defined peripherals on i2c-piix4 SMBus
Hi Enrico, On Thu, 8 Aug 2019 11:17:53 +0200, Enrico Weigelt, metux IT consult wrote: > On 02.08.19 14:51, Jean Delvare wrote: > > These patches fix a couple of issues with the i2c-piix4 driver on > > AMD Family 16h Model 30h SoCs and add ACPI-based enumeration to the > > i2c-piix4 driver. > > Can you tell a little bit more about what devices are behind the smbus ? > I recall the G-412 SoCs (such as on apu2+ boards) have an Hudson inside > and fall into this category. (I'll have to check when back in office), > so (as the apu2 platform driver maintainer) I'm very interested in this. Unfortunately not. I only picked up from where Andrew Cooks left, due to me being way too slow to review his patches. I did not want his work to be lost. I was able to test the first 2 patches which fix bugs, but not the 3rd one which deals with ACPI devices. There does not seem to be any such device on the 2 test machines I have remotely access to. > Does the probing need some special BIOS support (or do the necessary > table entries already come from aegesa) ? I assume that ACPI devices are declared in one of the ACPI tables, so it comes from the "BIOS", yes, whatever form it takes these days. > I have to admit, I'm still confused by the AMD documentation - haven't > found a clear documentation on what peripherals exactly are in the > G-412 SoC, just puzzled together that the FCH seems to be an Hudson, > probably v2. There also seems to be some relation between smbus and > gpio, but the gpio's are directly memory-mapped - no idea whether they > just share the same base address register or the gpios are really behind > smbus and some hw logic directy maps them into mmio space ... > Do you happen to have some more information on that ? I remember noticing long ago that SMBus ports were using GPIO pins, so these pins could be used for SMBus or for any other purpose. I could not find any way to figure out from the registers if a given pin pair was used for SMBus or not, which is pretty bad because it means we are blindly instantiating ALL possible SMBus ports even if some of the pins are used for a completely different purpose. It was over 1 year ago though, so I don't remember the details, and my findings then may not apply to the most recent hardware. > By the way: I'm considering collecting some hw documentation in the > kernel tree (maybe Documentation/hardware/...) - do you folks think > that's a good idea ? No. Only documentation specifically related to the Linux kernel should live in the kernel tree. OS-neutral documentation must go somewhere else. -- Jean Delvare SUSE L3 Support
[PATCH v5 2/3] i2c: piix4: Fix probing of reserved ports on AMD Family 16h Model 30h
Prevent bus timeouts and resets on Family 16h Model 30h by not probing
reserved Ports 3 and 4.
According to the AMD BIOS and Kernel Developer's Guides (BKDG), Port 3
and Port 4 are reserved on the following devices:
- Family 15h Model 60h-6Fh
- Family 15h Model 70h-7Fh
- Family 16h Model 30h-3Fh
Based on earlier work by Andrew Cooks.
Reported-by: Andrew Cooks
Signed-off-by: Jean Delvare
---
Changes since v4:
* Fix subject line
* Drop local variable port_count, use piix4_adapter_count
everywhere to represent the maximum number of main SMBus ports
* Exclude early Hudson2 implementations from this change
drivers/i2c/busses/i2c-piix4.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
--- linux-5.2.orig/drivers/i2c/busses/i2c-piix4.c 2019-08-02
14:08:17.003820098 +0200
+++ linux-5.2/drivers/i2c/busses/i2c-piix4.c2019-08-02 14:15:38.284423549
+0200
@@ -72,7 +72,8 @@
#define PIIX4_BLOCK_DATA 0x14
/* Multi-port constants */
-#define PIIX4_MAX_ADAPTERS 4
+#define PIIX4_MAX_ADAPTERS 4
+#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
/* SB800 constants */
#define SB800_PIIX4_SMB_IDX0xcd6
@@ -806,6 +807,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
static struct i2c_adapter *piix4_aux_adapter;
+static int piix4_adapter_count;
static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
bool sb800_main, u8 port, bool notify_imc,
@@ -865,7 +867,15 @@ static int piix4_add_adapters_sb800(stru
int port;
int retval;
- for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+ if (dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
+ (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
+dev->revision >= 0x1F)) {
+ piix4_adapter_count = HUDSON2_MAIN_PORTS;
+ } else {
+ piix4_adapter_count = PIIX4_MAX_ADAPTERS;
+ }
+
+ for (port = 0; port < piix4_adapter_count; port++) {
retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
piix4_main_port_names_sb800[port],
_main_adapters[port]);
@@ -987,7 +997,7 @@ static void piix4_adap_remove(struct i2c
static void piix4_remove(struct pci_dev *dev)
{
- int port = PIIX4_MAX_ADAPTERS;
+ int port = piix4_adapter_count;
while (--port >= 0) {
if (piix4_main_adapters[port]) {
--
Jean Delvare
SUSE L3 Support
[PATCH v5 3/3] i2c: piix4: Add ACPI support
Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
devices using ACPI. It builds on the related I2C mux device work
in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")
In the i2c-piix4 driver the adapters are enumerated as:
Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)
However, in the AMD BKDG documentation[1], the implied order of ports is:
Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...
This ordering difference is unfortunate. We assume that ACPI
developers will use the AMD documentation ordering, so we have to
pass an extra parameter to piix4_add_adapter().
[1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
Models 30h-3Fh Processors
Based on earlier work by Andrew Cooks.
Reported-by: Andrew Cooks
Signed-off-by: Jean Delvare
---
Changes since v4:
* Fix code alignment (reported by Tobin C. Harding)
* Adjust to changes in previous patch
* Use the SMBus numbering documented by AMD
drivers/i2c/busses/i2c-piix4.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
--- linux-5.2.orig/drivers/i2c/busses/i2c-piix4.c 2019-08-02
14:26:02.197346075 +0200
+++ linux-5.2/drivers/i2c/busses/i2c-piix4.c2019-08-02 14:40:40.990505068
+0200
@@ -811,7 +811,8 @@ static int piix4_adapter_count;
static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
bool sb800_main, u8 port, bool notify_imc,
-const char *name, struct i2c_adapter **padap)
+u8 hw_port_nr, const char *name,
+struct i2c_adapter **padap)
{
struct i2c_adapter *adap;
struct i2c_piix4_adapdata *adapdata;
@@ -843,6 +844,12 @@ static int piix4_add_adapter(struct pci_
/* set up the sysfs linkage to our parent device */
adap->dev.parent = >dev;
+ if (has_acpi_companion(>dev)) {
+ acpi_preset_companion(>dev,
+ ACPI_COMPANION(>dev),
+ hw_port_nr);
+ }
+
snprintf(adap->name, sizeof(adap->name),
"SMBus PIIX4 adapter%s at %04x", name, smba);
@@ -876,7 +883,10 @@ static int piix4_add_adapters_sb800(stru
}
for (port = 0; port < piix4_adapter_count; port++) {
+ u8 hw_port_nr = port == 0 ? 0 : port + 1;
+
retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
+ hw_port_nr,
piix4_main_port_names_sb800[port],
_main_adapters[port]);
if (retval < 0)
@@ -947,8 +957,8 @@ static int piix4_probe(struct pci_dev *d
return retval;
/* Try to register main SMBus adapter, give up if we can't */
- retval = piix4_add_adapter(dev, retval, false, 0, false, "",
- _main_adapters[0]);
+ retval = piix4_add_adapter(dev, retval, false, 0, false, 0,
+ "", _main_adapters[0]);
if (retval < 0)
return retval;
}
@@ -974,7 +984,7 @@ static int piix4_probe(struct pci_dev *d
if (retval > 0) {
/* Try to add the aux adapter if it exists,
* piix4_add_adapter will clean up if this fails */
- piix4_add_adapter(dev, retval, false, 0, false,
+ piix4_add_adapter(dev, retval, false, 0, false, 1,
is_sb800 ? piix4_aux_port_name_sb800 : "",
_aux_adapter);
}
--
Jean Delvare
SUSE L3 Support
[PATCH v5 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
From: Andrew Cooks
Family 16h Model 30h SMBus controller needs the same port selection fix
as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
selection for AMD Family 17h chips")
commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
register") also fixed the port selection for Hudson2, but unfortunately
this is not the exact same device and the AMD naming and PCI Device IDs
aren't particularly helpful here.
The SMBus port selection register is common to the following Families
and models, as documented in AMD's publicly available BIOS and Kernel
Developer Guides:
50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
between Bolton FCH and Family 16h Model 30h, but the location of the
SmBus0Sel port selection bits are different:
51192 - Bolton Register Reference Guide
We distinguish between Bolton and Family 16h Model 30h using the PCI
Revision ID:
Bolton is device 0x780b, revision 0x15
Family 16h Model 30h is device 0x780b, revision 0x1F
Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
The following additional public AMD BKDG documents were checked and do
not share the same port selection register:
42301 - Family 15h Model 00h-0Fh doesn't mention any
42300 - Family 15h Model 10h-1Fh doesn't mention any
49125 - Family 15h Model 30h-3Fh doesn't mention any
48751 - Family 16h Model 00h-0Fh uses the previously supported
index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
Signed-off-by: Andrew Cooks
Signed-off-by: Jean Delvare
Cc: sta...@vger.kernel.org [v4.6+]
---
Changes since v4:
* Removed unneeded parentheses (reported by Tobin C. Harding)
drivers/i2c/busses/i2c-piix4.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
--- linux-5.2.orig/drivers/i2c/busses/i2c-piix4.c 2019-07-28
21:57:05.337228192 +0200
+++ linux-5.2/drivers/i2c/busses/i2c-piix4.c2019-08-02 13:53:33.222597706
+0200
@@ -91,7 +91,7 @@
#define SB800_PIIX4_PORT_IDX_MASK 0x06
#define SB800_PIIX4_PORT_IDX_SHIFT 1
-/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
+/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
#define SB800_PIIX4_PORT_IDX_KERNCZ0x02
#define SB800_PIIX4_PORT_IDX_MASK_KERNCZ 0x18
#define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ 3
@@ -358,18 +358,16 @@ static int piix4_setup_sb800(struct pci_
/* Find which register is used for port selection */
if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD ||
PIIX4_dev->vendor == PCI_VENDOR_ID_HYGON) {
- switch (PIIX4_dev->device) {
- case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
+ if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
+ (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
+PIIX4_dev->revision >= 0x1F)) {
piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_KERNCZ;
piix4_port_mask_sb800 =
SB800_PIIX4_PORT_IDX_MASK_KERNCZ;
piix4_port_shift_sb800 =
SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ;
- break;
- case PCI_DEVICE_ID_AMD_HUDSON2_SMBUS:
- default:
+ } else {
piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
- break;
}
} else {
if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2,
--
Jean Delvare
SUSE L3 Support
[PATCH v5 0/3] Enable ACPI-defined peripherals on i2c-piix4 SMBus
These patches fix a couple of issues with the i2c-piix4 driver on AMD Family 16h Model 30h SoCs and add ACPI-based enumeration to the i2c-piix4 driver. Some I2C peripherals, eg. PCA953x IO expander, are not discovered by the probe or detect mechanisms when attached to an SMBus controller that uses the i2c-piix4 SMBus driver. ACPI provides a mechanism to define these peripherals and the controller port that they're attached to. Based on earlier work by Andrew Cooks. Changes: v5: take over from Andrew Cooks who apparently moved to other projects fix style issues reported by Tobin C. Harding fix potential array overrun make sure all registered adapters get unregistered keep ports 3 and 4 on early Hudson2 assume AMD SMBus numbering for ACPI devices v4: remove unnecessary SB800_MAIN_PORTS constant reduce piix4_remove change v3: take chip revision into account when determining port selection register v2: count the adapters, instead of misusing port numbers -- Jean Delvare SUSE L3 Support
NETIF_F_LLTX breaks iwlwifi
Hi Felix, Toke, Johannes, After updating to kernel 5.2, I started losing wireless network on my workstation a few minutes after boot. I could restart the network service to get it back, but it would go away again a few minutes later. No error message logged, but somehow the network traffic was no long being processed. My hardware is: 05:00.0 Network controller [0280]: Intel Corporation Wireless 8265 / 8275 [8086:24fd] (rev 78) This is an Intel 8265 PCIe WiFI adapter by Gigabyte, model GC-WB867D-I, which worked flawlessly for me until then. I bisected it down to: commit 8dbb000ee73be2c05e34756739ce308885312a29 (refs/bisect/bad) Author: Felix Fietkau Date: Sat Mar 16 18:06:34 2019 +0100 mac80211: set NETIF_F_LLTX when using intermediate tx queues So whatever the commit message says, it is apparently not safe to run TX handlers on multiple CPUs in parallel for this specific driver / device. Unless someone has an immediate explanation as to why it broke the iwlwifi driver and the actual bug is in iwlwifi and it can be fixed quickly and easily there, I would suggest that the above commit is reverted for the time being, as apparently it wasn't fixing anything but was just a performance optimization. I am available to do any amount of tests or debugging, given the guidance. Thanks, -- Jean Delvare SUSE L3 Support
[PATCH v2 2/2] nvmem: Use the same permissions for eeprom as for nvmem
The compatibility "eeprom" attribute is currently root-only no
matter what the configuration says. The "nvmem" attribute does
respect the setting of the root_only configuration bit, so do the
same for "eeprom".
Signed-off-by: Jean Delvare
Fixes: b6c217ab9be6 ("nvmem: Add backwards compatibility support for older
EEPROM drivers.")
Cc: Andrew Lunn
Cc: Srinivas Kandagatla
Cc: Greg Kroah-Hartman
Cc: Bartosz Golaszewski
Cc: Arnd Bergmann
---
Changes since V1:
* Split into 2 patches, one to the at24 driver and one to the nvmem
core. drivers/nvmem/nvmem-sysfs.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
--- linux-5.2.orig/drivers/nvmem/nvmem-sysfs.c 2019-07-08 00:41:56.0
+0200
+++ linux-5.2/drivers/nvmem/nvmem-sysfs.c 2019-07-28 18:06:53.105140893
+0200
@@ -224,10 +224,17 @@ int nvmem_sysfs_setup_compat(struct nvme
if (!config->base_dev)
return -EINVAL;
- if (nvmem->read_only)
- nvmem->eeprom = bin_attr_ro_root_nvmem;
- else
- nvmem->eeprom = bin_attr_rw_root_nvmem;
+ if (nvmem->read_only) {
+ if (config->root_only)
+ nvmem->eeprom = bin_attr_ro_root_nvmem;
+ else
+ nvmem->eeprom = bin_attr_ro_nvmem;
+ } else {
+ if (config->root_only)
+ nvmem->eeprom = bin_attr_rw_root_nvmem;
+ else
+ nvmem->eeprom = bin_attr_rw_nvmem;
+ }
nvmem->eeprom.attr.name = "eeprom";
nvmem->eeprom.size = nvmem->size;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
--
Jean Delvare
SUSE L3 Support
[PATCH v2 1/2] eeprom: at24: make spd world-readable again
The integration of the at24 driver into the nvmem framework broke the
world-readability of spd EEPROMs. Fix it.
Signed-off-by: Jean Delvare
Fixes: 57d155506dd5 ("eeprom: at24: extend driver to plug into the NVMEM
framework")
Cc: Andrew Lunn
Cc: Srinivas Kandagatla
Cc: Greg Kroah-Hartman
Cc: Bartosz Golaszewski
Cc: Arnd Bergmann
---
Note: This is only the 1st half of the fix, the nvmem core driver
also needs to be fixed.
Changes since V1:
* Split into 2 patches, one to the at24 driver and one to the nvmem
core.
drivers/misc/eeprom/at24.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-5.1.orig/drivers/misc/eeprom/at24.c 2019-07-28 16:52:06.550918923
+0200
+++ linux-5.1/drivers/misc/eeprom/at24.c2019-07-28 16:53:28.104167083
+0200
@@ -719,7 +719,7 @@ static int at24_probe(struct i2c_client
nvmem_config.name = dev_name(dev);
nvmem_config.dev = dev;
nvmem_config.read_only = !writable;
- nvmem_config.root_only = true;
+ nvmem_config.root_only = !(flags & AT24_FLAG_IRUGO);
nvmem_config.owner = THIS_MODULE;
nvmem_config.compat = true;
nvmem_config.base_dev = dev;
--
Jean Delvare
SUSE L3 Support
Re: [RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support
My attempt looks like that:
Based on earlier work by Andrew Cooks.
Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
devices using ACPI. It builds on the related I2C mux device work
in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")
In the i2c-piix4 driver the adapters are enumerated as:
Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)
However, in the AMD BKDG documentation[1], the implied order of ports is:
Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...
This ordering difference is unfortunate. We assume that ACPI
developers will use the AMD documentation ordering, so we have to
pass an extra parameter to piix4_add_adapter().
[1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
Models 30h-3Fh Processors
Signed-off-by: Jean Delvare
---
drivers/i2c/busses/i2c-piix4.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
--- linux-5.1.orig/drivers/i2c/busses/i2c-piix4.c 2019-07-24
11:29:49.836450493 +0200
+++ linux-5.1/drivers/i2c/busses/i2c-piix4.c2019-07-24 15:21:20.166362730
+0200
@@ -819,7 +819,8 @@ static int piix4_adapter_count;
static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
bool sb800_main, u8 port, bool notify_imc,
-const char *name, struct i2c_adapter **padap)
+u8 hw_port_nr, const char *name,
+struct i2c_adapter **padap)
{
struct i2c_adapter *adap;
struct i2c_piix4_adapdata *adapdata;
@@ -851,6 +852,12 @@ static int piix4_add_adapter(struct pci_
/* set up the sysfs linkage to our parent device */
adap->dev.parent = >dev;
+ if (has_acpi_companion(>dev)) {
+ acpi_preset_companion(>dev,
+ ACPI_COMPANION(>dev),
+ hw_port_nr);
+ }
+
snprintf(adap->name, sizeof(adap->name),
"SMBus PIIX4 adapter%s at %04x", name, smba);
@@ -883,7 +890,10 @@ static int piix4_add_adapters_sb800(stru
}
for (port = 0; port < piix4_adapter_count; port++) {
+ u8 hw_port_nr = port == 0 ? 0 : port + 1;
+
retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
+ hw_port_nr,
piix4_main_port_names_sb800[port],
_main_adapters[port]);
if (retval < 0)
@@ -954,8 +964,8 @@ static int piix4_probe(struct pci_dev *d
return retval;
/* Try to register main SMBus adapter, give up if we can't */
- retval = piix4_add_adapter(dev, retval, false, 0, false, "",
- _main_adapters[0]);
+ retval = piix4_add_adapter(dev, retval, false, 0, false, 0,
+ "", _main_adapters[0]);
if (retval < 0)
return retval;
}
@@ -981,7 +991,7 @@ static int piix4_probe(struct pci_dev *d
if (retval > 0) {
/* Try to add the aux adapter if it exists,
* piix4_add_adapter will clean up if this fails */
- piix4_add_adapter(dev, retval, false, 0, false,
+ piix4_add_adapter(dev, retval, false, 0, false, 1,
is_sb800 ? piix4_aux_port_name_sb800 : "",
_aux_adapter);
}
--
Jean Delvare
SUSE L3 Support
Re: [RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support
On Mon, 26 Feb 2018 10:28:45 +1000, Andrew Cooks wrote:
> Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
> devices using ACPI. It builds on the related I2C mux device work
> in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")
>
> In the i2c-piix4 driver the adapters are enumerated as:
> Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)
>
> However, in the AMD BKDG documentation[1], the implied order of ports is:
> Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...
>
> This ordering difference is unfortunate. We assume that ACPI
> developers will use the Linux ordering.
>
> [1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
> Models 30h-3Fh Processors
>
> Signed-off-by: Andrew Cooks
> ---
> drivers/i2c/busses/i2c-piix4.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 01f1610..9a6cdc8 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -837,6 +837,12 @@ static int piix4_add_adapter(struct pci_dev *dev,
> unsigned short smba,
> /* set up the sysfs linkage to our parent device */
> adap->dev.parent = >dev;
>
> + if (has_acpi_companion(>dev)) {
> + acpi_preset_companion(>dev,
> + ACPI_COMPANION(>dev),
> + piix4_adapter_count);
After the change I proposed for the previous patch, this is no longer
going to work. But I don't think it was really working before anyway.
For one thing, for the same reason I want to change the previous patch:
in case of failure to register some of the adapters, the numbering of
later adapters would be shifted. Also giving the aux bus a different
number depending on the device (4 before Hudson2, 2 for Hudson2 and
later) is unlikely to match the BIOS expectations.
For another, the assumption that "ACPI developers will use the Linux
ordering" is unlikely to be valid. I think we are talking about BIOS
developers here, and they should be OS-agnostic. If they are not, then
most likely they would align with whatever Windows drivers expect. So
our best chance is to stick to the datasheet.
Lastly, this would be inconsistent even with our own driver. We are
indeed not instantiating the adapters in the order listed by the
datasheet, and i2c adapter numbering is dynamic, but we are *naming* the
adapters to match the datasheet. So we should really pass the same
number to the ACPI layer, for consistency if nothing else. This
probably means passing one more parameter to piix4_add_adapter() and/or
some more code to figure out the bus number to pass to
acpi_preset_companion(), but I don't think we can avoid that, so let's
just do it.
> + }
> +
> snprintf(adap->name, sizeof(adap->name),
> "SMBus PIIX4 adapter%s at %04x", name, smba);
>
--
Jean Delvare
SUSE L3 Support
Re: [RESEND][PATCH v4 2/3] i2c: piix4: fix probing of reserved ports on AMD
nstants */
-#define PIIX4_MAX_ADAPTERS 4
+#define PIIX4_MAX_ADAPTERS 4
+#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
/* SB800 constants */
#define SB800_PIIX4_SMB_IDX0xcd6
@@ -814,6 +815,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
static struct i2c_adapter *piix4_aux_adapter;
+static int piix4_adapter_count;
static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
bool sb800_main, u8 port, bool notify_imc,
@@ -873,7 +875,14 @@ static int piix4_add_adapters_sb800(stru
int port;
int retval;
- for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+ if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
+ dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
+ piix4_adapter_count = HUDSON2_MAIN_PORTS;
+ } else {
+ piix4_adapter_count = PIIX4_MAX_ADAPTERS;
+ }
+
+ for (port = 0; port < piix4_adapter_count; port++) {
retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
piix4_main_port_names_sb800[port],
_main_adapters[port]);
@@ -995,7 +1005,7 @@ static void piix4_adap_remove(struct i2c
static void piix4_remove(struct pci_dev *dev)
{
- int port = PIIX4_MAX_ADAPTERS;
+ int port = piix4_adapter_count;
while (--port >= 0) {
if (piix4_main_adapters[port]) {
Anyone sees a problem with this approach?
The second issue I see is that ports 3 and 4 are skipped for all
PCI_DEVICE_ID_AMD_HUDSON2_SMBUS devices while the previous patch in
this series suggests that the Bolton chipset (device revision 0x15)
behaves differently. As a matter of fact the description of this patch
does not list it as being affected by the issue. So in effect we would
be disabling ports 3 and 4 for Bolton users, who may have valuable I2C
devices connected there. We just fixed a regression for them with
previous patch, let's not introduce a new one with this patch. I think
the device test should look like:
if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
(PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
PIIX4_dev->revision >= 0x1F)) {
piix4_adapter_count = HUDSON2_MAIN_PORTS;
} else {
(...)
i.e. the same as in previous patch.
By the way, I don't own compatible hardware. I will see if I can gain
access to a SUSE Labs machine for testing purposes, but it would be a
lot easier if someone with the hardware could test the patch series
once it is rebased. Volunteers?
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
On Wed, 24 Jul 2019 10:37:48 +0200, Jean Delvare wrote: > Hi Andrew, > > Sorry for the delay... What can I say :( Unfortunately by now Andrew is gone. So I will be the one rebasing and resubmitting this series. -- Jean Delvare SUSE L3 Support
Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
Hi Andrew,
Sorry for the delay... What can I say :(
On Mon, 26 Feb 2018 10:28:43 +1000, Andrew Cooks wrote:
> Family 16h Model 30h SMBus controller needs the same port selection fix
> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
> selection for AMD Family 17h chips")
>
> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
> register") also fixed the port selection for Hudson2, but unfortunately
> this is not the exact same device and the AMD naming and PCI Device IDs
> aren't particularly helpful here.
>
> The SMBus port selection register is common to the following Families
> and models, as documented in AMD's publicly available BIOS and Kernel
> Developer Guides:
>
> 50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
>
> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
> between Bolton FCH and Family 16h Model 30h, but the location of the
> SmBus0Sel port selection bits are different:
>
> 51192 - Bolton Register Reference Guide
>
> We distinguish between Bolton and Family 16h Model 30h using the PCI
> Revision ID:
>
> Bolton is device 0x780b, revision 0x15
> Family 16h Model 30h is device 0x780b, revision 0x1F
> Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
>
> The following additional public AMD BKDG documents were checked and do
> not share the same port selection register:
>
> 42301 - Family 15h Model 00h-0Fh doesn't mention any
> 42300 - Family 15h Model 10h-1Fh doesn't mention any
> 49125 - Family 15h Model 30h-3Fh doesn't mention any
>
> 48751 - Family 16h Model 00h-0Fh uses the previously supported
> index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
>
> Signed-off-by: Andrew Cooks
> ---
> drivers/i2c/busses/i2c-piix4.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
> (...)
Looks good to me. Unfortunately the patch no longer applies (my fault
obviously), it needs to be rebased on top of commit
24beb83ad289c68bce7c01351cb90465bbb1940a ("i2c-piix4: Add Hygon Dhyana
SMBus support").
I also agree with Tobin's suggestion to remove unneeded parentheses.
Reviewed-by: Jean Delvare
This patch should also address Will Wagner's (Cc'd) complaint in another
thread ("[BUG] i2c_piix4: Hudson2 uses wrong port to access SMBus
controller").
I believe this is stable branch material.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: busses: Use dev_get_drvdata where possible
On Tue, 23 Jul 2019 19:11:10 +0800, Chuhong Yuan wrote: > Instead of using to_pci_dev + pci_get_drvdata, > use dev_get_drvdata to make code simpler. > > Signed-off-by: Chuhong Yuan > --- > drivers/i2c/busses/i2c-designware-pcidrv.c | 6 ++ > drivers/i2c/busses/i2c-i801.c | 3 +-- > 2 files changed, 3 insertions(+), 6 deletions(-) > (...) Looks good to me, thanks. Reviewed-by: Jean Delvare -- Jean Delvare SUSE L3 Support
[PATCH 3/3] soc: ixp4xx: Hide auto-selected drivers
The 2 IXP4xx SOC drivers qmgr and npe are selected automatically when needed so they do not need to be presented to the user. Furthermore these helper drivers are specific to the IXP4xx arch so hide them completely on other architectures so as to not clutter the config file. Signed-off-by: Jean Delvare Cc: Krzysztof Halasa Cc: Linus Walleij --- Note: Ultimately all I want is that non-ixp4xx users are not asked about IXP4XX_QMGR and IXP4XX_NPE. If there is another preferred option to achieve the same, that would work with me too, just let me know. drivers/soc/ixp4xx/Kconfig |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) --- linux-5.2.orig/drivers/soc/ixp4xx/Kconfig 2019-07-08 00:41:56.0 +0200 +++ linux-5.2/drivers/soc/ixp4xx/Kconfig2019-07-12 15:21:44.769229835 +0200 @@ -1,17 +1,19 @@ # SPDX-License-Identifier: GPL-2.0-only +if ARCH_IXP4XX menu "IXP4xx SoC drivers" config IXP4XX_QMGR - tristate "IXP4xx Queue Manager support" + tristate help This driver supports IXP4xx built-in hardware queue manager and is automatically selected by Ethernet and HSS drivers. config IXP4XX_NPE - tristate "IXP4xx Network Processor Engine support" + tristate select FW_LOADER help This driver supports IXP4xx built-in network coprocessors and is automatically selected by Ethernet and HSS drivers. endmenu +endif -- Jean Delvare SUSE L3 Support
[PATCH 2/3] soc: ixp4xx: Really select helper drivers automatically
Kconfig claims that the ixp4xx-qmgr and ixp4xx-npe helper drivers are selected automatically as needed. However this is not what the Kconfig entries are doing. Convert depends to select to match the help texts. Signed-off-by: Jean Delvare Cc: Krzysztof Halasa --- Sorry about the weird patch numbering, I found this issue right before sending the other patch so I had to insert a new patch in the middle of the series. This should get tested on ixp4xx, I could only test on x86. drivers/net/ethernet/xscale/Kconfig |7 --- drivers/net/wan/Kconfig |4 +++- 2 files changed, 7 insertions(+), 4 deletions(-) --- linux-5.2.orig/drivers/net/ethernet/xscale/Kconfig 2019-07-12 15:44:22.617698231 +0200 +++ linux-5.2/drivers/net/ethernet/xscale/Kconfig 2019-07-12 15:48:00.538938930 +0200 @@ -6,8 +6,7 @@ config NET_VENDOR_XSCALE bool "Intel XScale IXP devices" default y - depends on NET_VENDOR_INTEL && (ARM && ARCH_IXP4XX && \ - IXP4XX_NPE && IXP4XX_QMGR) + depends on NET_VENDOR_INTEL && (ARM && ARCH_IXP4XX) ---help--- If you have a network (Ethernet) card belonging to this class, say Y. @@ -20,9 +19,11 @@ if NET_VENDOR_XSCALE config IXP4XX_ETH tristate "Intel IXP4xx Ethernet support" - depends on ARM && ARCH_IXP4XX && IXP4XX_NPE && IXP4XX_QMGR + depends on ARM && ARCH_IXP4XX select PHYLIB select NET_PTP_CLASSIFY + select IXP4XX_NPE + select IXP4XX_QMGR ---help--- Say Y here if you want to use built-in Ethernet ports on IXP4xx processor. --- linux-5.2.orig/drivers/net/wan/Kconfig 2019-07-12 15:44:22.628698395 +0200 +++ linux-5.2/drivers/net/wan/Kconfig 2019-07-12 15:47:25.211414758 +0200 @@ -329,7 +329,9 @@ config DSCC4_PCI_RST config IXP4XX_HSS tristate "Intel IXP4xx HSS (synchronous serial port) support" - depends on HDLC && ARM && ARCH_IXP4XX && IXP4XX_NPE && IXP4XX_QMGR + depends on HDLC && ARM && ARCH_IXP4XX + select IXP4XX_NPE + select IXP4XX_QMGR help Say Y here if you want to use built-in HSS ports on IXP4xx processor. -- Jean Delvare SUSE L3 Support
[PATCH 1/2] soc: ixp4xx: List the whole directory in MAINTAINERS
Mention the whole directory containing the ixp4xx soc drivers in MAINTAINERS instead of each driver separately. Otherwise changes done to Makefile and Kconfig will fail to find a matching entry. This will also let future drivers match without having to update the entry each time. Signed-off-by: Jean Delvare Cc: Krzysztof Halasa --- MAINTAINERS |6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) --- linux-5.2.orig/MAINTAINERS 2019-07-12 15:33:28.106852821 +0200 +++ linux-5.2/MAINTAINERS 2019-07-12 15:35:54.309067537 +0200 @@ -8023,10 +8023,8 @@ F: Documentation/media/v4l-drivers/ipu3. INTEL IXP4XX QMGR, NPE, ETHERNET and HSS SUPPORT M: Krzysztof Halasa S: Maintained -F: include/linux/soc/ixp4xx/qmgr.h -F: include/linux/soc/ixp4xx/npe.h -F: drivers/soc/ixp4xx/ixp4xx-qmgr.c -F: drivers/soc/ixp4xx/ixp4xx-npe.c +F: include/linux/soc/ixp4xx/ +F: drivers/soc/ixp4xx/ F: drivers/net/ethernet/xscale/ixp4xx_eth.c F: drivers/net/wan/ixp4xx_hss.c -- Jean Delvare SUSE L3 Support
[PATCH] staging: kpc2000: drop useless softdep statement
The i2c-dev module is for access to I2C buses from user-space.
Kernel drivers do not care about its presence.
Signed-off-by: Jean Delvare
Cc: Matt Sickler
Cc: Greg Kroah-Hartman
---
drivers/staging/kpc2000/kpc_i2c/i2c_driver.c |1 -
1 file changed, 1 deletion(-)
--- linux-5.2-rc7.orig/drivers/staging/kpc2000/kpc_i2c/i2c_driver.c
2019-06-30 05:25:36.0 +0200
+++ linux-5.2-rc7/drivers/staging/kpc2000/kpc_i2c/i2c_driver.c 2019-07-01
11:33:17.336697545 +0200
@@ -26,7 +26,6 @@
MODULE_LICENSE("GPL");
MODULE_AUTHOR("matt.sick...@daktronics.com");
-MODULE_SOFTDEP("pre: i2c-dev");
struct i2c_device {
unsigned long smba;
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v5] i2c: i801: Register optional lis3lv02d I2C device on Dell machines
On Thu, 6 Jun 2019 20:18:45 +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
>
> ---
> Changes since v4:
> * Remove usage of redundant acpi_bus_get_status_handle()
> * Update comment about acpi_get_devices()
> (...)
Looks all good now.
Reviewed-by: Jean Delvare
Thanks!
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v4] i2c: i801: Register optional lis3lv02d I2C device on Dell machines
On Thu, 6 Jun 2019 17:48:00 +0200, Pali Rohár wrote: > On Thursday 06 June 2019 16:53:09 Jean Delvare wrote: > > This is testing that *either* bit is set. Is it what you intend to > > achieve, or would you rather want to ensure that *all* these bits are > > set? > > Of course, it is wrong. Thanks for catch. We should ignore apci devices > which are not present, which are disabled or which are not functioning. > > Now I looked into acpi_get_devices() implementation and it call > acpi_ns_get_device_callback() function callback for every device. At the > end that function calls user supplied check_acpi_smo88xx_device > function. > > And acpi_ns_get_device_callback() already ignores acpi devices which do > not have ACPI_STA_DEVICE_PRESENT or ACPI_STA_DEVICE_FUNCTIONING flag. > > According to acpi documentation when ACPI_STA_DEVICE_PRESENT is not set > then ACPI_STA_DEVICE_ENABLED also cannot be set. > > So the whole acpi_bus_get_status_handle() is not needed at all as > acpi_get_devices() via acpi_ns_get_device_callback() already filter > unsuitable acpi devices. > > I guess that I already did this investigation in past and added comment > "exists and is enabled" which is below near acpi_get_devices() call. But > as I wrote this patch more then year ago I forgot about it. Sorry for confusing you :-( > I will remove that check. Do you have any suggestion what to write into > comment so other readers in future would know that we do not need to > check anything with _STA acpi method? Well, if you manage to squeeze a short version of the above explanation at the relevant place in the driver, that should work. Doesn't have to be verbose really, anything that says that disabled devices will be properly skipped for us will be good enough. -- Jean Delvare SUSE L3 Support
Re: [PATCH v4] i2c: i801: Register optional lis3lv02d I2C device on Dell machines
Hi Pali,
On Wed, 5 Jun 2019 00:33: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
>
> ---
> Changes since v3:
> * Use char * [] type for list of acpi ids
> * Check that SMO88xx acpi device is present, enabled and functioning
> * Simplify usage of acpi_get_devices()
> * Change i2c to I2C
> * Make dell_lis3lv02d_devices const
>
> 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 | 123
>
> drivers/platform/x86/dell-smo8800.c | 1 +
> 2 files changed, 124 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ac7f7817dc89..9060d4b16f4f 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1134,6 +1134,126 @@ 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 char *const acpi_smo8800_ids[] = {
> + "SMO8800",
> + "SMO8801",
> + "SMO8810",
> + "SMO8811",
> + "SMO8820",
> + "SMO8821",
> + "SMO8830",
> + "SMO8831",
> +};
> +
> +static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> + u32 nesting_level,
> + void *context,
> + void **return_value)
> +{
> + struct acpi_device_info *info;
> + unsigned long long sta;
> + acpi_status status;
> + char *hid;
> + int i;
> +
> + status = acpi_bus_get_status_handle(obj_handle, );
> + if (!ACPI_SUCCESS(status))
> + return AE_OK;
> + if (!(sta & (ACPI_STA_DEVICE_PRESENT |
> + ACPI_STA_DEVICE_ENABLED |
> + ACPI_STA_DEVICE_FUNCTIONING)))
> + return AE_OK;
This is testing that *either* bit is set. Is it what you intend to
achieve, or would you rather want to ensure that *all* these bits are
set?
> +
> + status = acpi_get_object_info(obj_handle, );
> + 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]) == 0) {
> + *((bool *)return_value) = true;
> + return AE_CTRL_TERMINATE;
> + }
> + }
> +
> + return AE_OK;
> +}
> +
> +static bool is_dell_system_with_lis3lv02d(void)
> +{
> + bool found;
> + 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
> + * device represent our ST microelectronics lis3lv02d accelerometer but
> + * unfortunately without any other information (like I2C address).
> + */
> + found = false;
> + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> + (void **));
Alignment is incorrect now - but don't resend just for this.
> +
> + return found;
> +}
> (...)
Everything else looks good to me now. Has the latest version of your
patch been tested on real hardware?
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v3] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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(>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.
> + return;
> + }
> + (...)
All the rest looks good to me.
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tue, 28 May 2019 11:54:02 +0200, Pali Rohár wrote: > On Tuesday 28 May 2019 11:50:15 Jean Delvare wrote: > > OK, thanks for the explanation. But assuming that we now instantiate > > the lis2lv02d device from i2c-i801 for exactly all the same machines, > > can't we just *enable* the freefall misc device feature of lis2lv02d > > and kill the dell-smo8800 driver completely? Seems more simple to > > maintain going forward. > > I though about it and I already wrote that is it not practical. For ACPI > drivers there is easy way to get that interrupt number from ACPI tables. > From i2c-i801 PCI driver it is hard to get interrupt number for > particular ACPI device... > > That is way I preferred simple solution: ACPI driver for ACPI device and > i2c driver for i2c device. OK, fine with me then :-) -- Jean Delvare SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tue, 2019-05-28 at 11:41 +0200, Pali Rohár wrote: > This is not a problem. lis3lv02d provides two things: > > 1) 3 axes accelerometer > 2) optional interrupt and signal it to userspace via misc device > > dell-smo8800 does not call any parts of lis2lv02d module. It just > provides for userspace same misc device API as lis3lv02d. > > As lis3lv02d has misc device optional, registered i2c device from > i2c-i801 does not enable it. > > So technically it is one device, but their functionality divided into > two modules. One which reports accelerometer axes and one which signals > disk fall interrupt. These two modules and functionalities does not have > to interact, so it is safe to have them separated. OK, thanks for the explanation. But assuming that we now instantiate the lis2lv02d device from i2c-i801 for exactly all the same machines, can't we just *enable* the freefall misc device feature of lis2lv02d and kill the dell-smo8800 driver completely? Seems more simple to maintain going forward. -- Jean Delvare SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Mon, 26 Feb 2018 21:32:55 +0100, Wolfram Sang wrote: > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > Therefore instructing future contributors would be up to them. > > This is really Jean's realm. Sorry for the delay. As a general rule I'm all in favor of instantiating I2C devices from i2c-i801 when we can, as it makes the user's life easier. However I agree with Andy that: 1* We want to have an explicit list of supported ACPI device IDs, not a just a prefix. 2* We don't want to over-engineer it with a common header file or an exported symbol. I see no problem with duplicating the lists if 2 drivers happen to be needed on the same set of devices. This is easily managed by adding a comment before each list that the other list may need to be kept in sync. It also gives us the flexibility to *not* keep them in sync if needed. Instantiating the I2C device from dell-smo8800 doesn't seem practical because that driver has no idea about the i2c subsystem in the first place. What worries me is that we seem to have 2 drivers binding to the same device (the accelerometer), one natively (lis3lv02d), and one through an ACPI layer (dell-smo8800). I don't really understand why this is needed (don't they serve the same purpose?) nor how it can be safe (what guarantees that both drivers won't attempt to access the hardware at the same time?) -- Jean Delvare SUSE L3 Support
Re: [PATCH 3/3] i2c: i801: avoid panic if ioreamp fails
On Fri, 10 May 2019 17:35:46 +0800, Kefeng Wang wrote: > On 2019/5/10 16:09, Jean Delvare wrote: > > We don't need this anyway. The comment says it can't fail, so why > > bother checking for a condition which will never happen? > > The ioremap could fails due to no memory, our inner test robot(enable > FAULT_INJECTION) > > find this issue. The code only runs on x86 where this specific memory segment is standardized for the purpose. That's how we know it "can't fail". That being said, maybe it could fail for other reasons (internal kernel bug, or bogus BIOS maybe), and I don't care adding the check anyway, as this code path is not performance critical. -- Jean Delvare SUSE L3 Support
Re: [PATCH 3/3] i2c: i801: avoid panic if ioreamp fails
Hi Kefeng,
On Fri, 10 May 2019 11:03:20 +0800, Kefeng Wang wrote:
> If ioremap fails, NULL pointer dereference will happen and
> leading to a kernel panic when access the virtual address
> in check_signature().
>
> Fix it by check the return value of ioremap.
>
> Cc: Jean Delvare
> Cc: Wolfram Sang
> Cc: linux-...@vger.kernel.org
> Reported-by: Hulk Robot
> Signed-off-by: Kefeng Wang
> ---
> drivers/i2c/busses/i2c-i801.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 679c6c41f64b..fc6ccb8aba17 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1068,7 +1068,10 @@ static void __init input_apanel_init(void)
> void __iomem *bios;
> const void __iomem *p;
>
> - bios = ioremap(0xF, 0x1); /* Can't fail */
> + bios = ioremap(0xF, 0x1);
> + if (!base)
That would be "if (!bios)". Please don't send patches without at least
test-building the result.
We don't need this anyway. The comment says it can't fail, so why
bother checking for a condition which will never happen?
> + return -ENOMEM;
> +
> p = bios_signature(bios);
> if (p) {
> /* just use the first address */
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: Allow selecting BCM2835 I2C controllers on ARCH_BRCMSTB
On Thu, 9 May 2019 14:04:36 -0700, Florian Fainelli wrote: > From: Kamal Dasu > > ARCH_BRCMSTB platforms have the BCM2835 I2C controllers, allow > selecting the i2c-bcm2835 driver on such platforms. > > Signed-off-by: Kamal Dasu > Signed-off-by: Florian Fainelli > --- > drivers/i2c/busses/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 26186439db6b..7277c1051ca2 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -435,7 +435,7 @@ config I2C_AXXIA > > config I2C_BCM2835 > tristate "Broadcom BCM2835 I2C controller" > - depends on ARCH_BCM2835 > + depends on ARCH_BCM2835 || ARCH_BRCMSTB > help > If you say yes to this option, support will be included for the > BCM2835 I2C controller. Reviewed-by: Jean Delvare Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH 2/2] eeprom: ee1004: Deal with nack on page selection
On Mon, 6 May 2019 17:03:20 +0300, Jarkko Nikula wrote: > On 5/6/19 4:16 PM, Jean Delvare wrote: > > Some EE1004 implementations will not properly ack page selection > > commands. They still set the page correctly, so there is no actual > > error. Deal with this case gracefully by checking the currently > > selected page after we receive a nack. If the page is set right then > > we can continue. > > > > Signed-off-by: Jean Delvare > > Tested-by: Jarkko Nikula > > Cc: Arnd Bergmann > > Cc: Greg Kroah-Hartman > > --- > > drivers/misc/eeprom/ee1004.c | 12 +++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > Does Dreamcat4 deserve reported and tested by tags here? I guess > anonymous address is fine with those tags? My assumption is that someone who posts anonymously in the first place has no desire to be credited for anything or even mentioned anywhere. > (I re-tested these two patches on top of v5.1 and they make decode-dimms > working on a machine with 2-4 * Crucial DD4 dimms) Thank you very much. -- Jean Delvare SUSE L3 Support
[PATCH 2/2] eeprom: ee1004: Deal with nack on page selection
Some EE1004 implementations will not properly ack page selection
commands. They still set the page correctly, so there is no actual
error. Deal with this case gracefully by checking the currently
selected page after we receive a nack. If the page is set right then
we can continue.
Signed-off-by: Jean Delvare
Tested-by: Jarkko Nikula
Cc: Arnd Bergmann
Cc: Greg Kroah-Hartman
---
drivers/misc/eeprom/ee1004.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
--- linux-5.0.orig/drivers/misc/eeprom/ee1004.c 2019-05-06 11:57:14.333572368
+0200
+++ linux-5.0/drivers/misc/eeprom/ee1004.c 2019-05-06 15:11:06.009718220
+0200
@@ -1,7 +1,7 @@
/*
* ee1004 - driver for DDR4 SPD EEPROMs
*
- * Copyright (C) 2017 Jean Delvare
+ * Copyright (C) 2017-2019 Jean Delvare
*
* Based on the at24 driver:
* Copyright (C) 2005-2007 David Brownell
@@ -124,6 +124,16 @@ static ssize_t ee1004_read(struct file *
/* Data is ignored */
status = i2c_smbus_write_byte(ee1004_set_page[page],
0x00);
+ if (status == -ENXIO) {
+ /*
+* Don't give up just yet. Some memory
+* modules will select the page but not
+* ack the command. Check which page is
+* selected now.
+*/
+ if (ee1004_get_current_page() == page)
+ status = 0;
+ }
if (status < 0) {
dev_err(dev, "Failed to select page %d (%d)\n",
page, status);
--
Jean Delvare
SUSE L3 Support
[PATCH 1/2] eeprom: ee1004: Move selected page detection to a separate function
No functional change, this is in preparation for future needs.
Signed-off-by: Jean Delvare
Tested-by: Jarkko Nikula
Cc: Arnd Bergmann
Cc: Greg Kroah-Hartman
---
drivers/misc/eeprom/ee1004.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
--- linux-5.0.orig/drivers/misc/eeprom/ee1004.c 2019-05-06 11:23:14.833319076
+0200
+++ linux-5.0/drivers/misc/eeprom/ee1004.c 2019-05-06 11:56:58.478375343
+0200
@@ -57,6 +57,24 @@ MODULE_DEVICE_TABLE(i2c, ee1004_ids);
/*-*/
+static int ee1004_get_current_page(void)
+{
+ int err;
+
+ err = i2c_smbus_read_byte(ee1004_set_page[0]);
+ if (err == -ENXIO) {
+ /* Nack means page 1 is selected */
+ return 1;
+ }
+ if (err < 0) {
+ /* Anything else is a real error, bail out */
+ return err;
+ }
+
+ /* Ack means page 0 is selected, returned value meaningless */
+ return 0;
+}
+
static ssize_t ee1004_eeprom_read(struct i2c_client *client, char *buf,
unsigned int offset, size_t count)
{
@@ -190,17 +208,10 @@ static int ee1004_probe(struct i2c_clien
}
/* Remember current page to avoid unneeded page select */
- err = i2c_smbus_read_byte(ee1004_set_page[0]);
- if (err == -ENXIO) {
- /* Nack means page 1 is selected */
- ee1004_current_page = 1;
- } else if (err < 0) {
- /* Anything else is a real error, bail out */
+ err = ee1004_get_current_page();
+ if (err < 0)
goto err_clients;
- } else {
- /* Ack means page 0 is selected, returned value meaningless */
- ee1004_current_page = 0;
- }
+ ee1004_current_page = err;
dev_dbg(>dev, "Currently selected page: %d\n",
ee1004_current_page);
mutex_unlock(_bus_lock);
--
Jean Delvare
SUSE L3 Support
Re: [RFC PATCH v2] i2c-piix4: Add Hygon Dhyana SMBus support
On Sat, 27 Apr 2019 09:07:44 +0800, Pu Wen wrote: > The Hygon Dhyana CPU has the SMBus device with PCI device ID 0x790b, > which is the same as AMD CZ SMBus device. So add Hygon Dhyana support > to the i2c-piix4 driver by using the code path of AMD. > > Signed-off-by: Pu Wen > --- > v1->v2: > - Remove the revision number checking for Hygon SMBus device. > - Document the new supported chipset in drivers/i2c/busses/Kconfig > and Documentation/i2c/busses/i2c-piix4 as well as in the header > comment of i2c-piix4.c. > > Documentation/i2c/busses/i2c-piix4 | 2 ++ > drivers/i2c/busses/Kconfig | 1 + > drivers/i2c/busses/i2c-piix4.c | 15 +++ > 3 files changed, 14 insertions(+), 4 deletions(-) > (...) Looks good to me now, thanks. Reviewed-by: Jean Delvare -- Jean Delvare SUSE L3 Support
Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2
On Mon, 2019-04-29 at 10:53 +0200, Benjamin Tissoires wrote: > On Mon, Apr 29, 2019 at 10:38 AM Jarkko Nikula > wrote: > > I got another thought about this. I noticed these input drivers need > > SMBus Host Notify, maybe that explain the PM dependency? If that's the > > only dependency then we could prevent the controller suspend if there is > > a client needing host notify mechanism. IMHO that's less hack than the > > patch to rmi_smbus.c. > > So currently, AFAIK, only Synaptics (rmi4) and Elantech are using > SMBus Host Notify. > So this patch would prevent the same bugs for those 2 vendors, which is good. > > It took me some time to understand why this would be less than a hack. > And indeed, given that Host Notify relies on the I2C connection to be > ready for the IRQ, we can not put the controller in suspend like we do > for others where the IRQ controller is still ready. > > So yes, that could work from me. Not sure what Wolfram and Jean would > say though. I would say OK with me, this looks like the cleanest solution to me, so if testing is positive, let's go with it. -- Jean Delvare SUSE L3 Support
Re: [RFC PATCH RESEND] i2c-piix4: Add Hygon Dhyana SMBus support
On Fri, 2019-04-26 at 22:23 +0800, Pu Wen wrote: > On 2019/4/26 17:38, Jean Delvare wrote: > > I would like you to also document the new supported chipset in > > drivers/i2c/busses/Kconfig and Documentation/i2c/busses/i2c-piix4 as > > well as in the header comment of i2c-piix4.c itself. I know it seems > > redundant but it helps the user know which driver they need. > > Because Hygon uses the same PCI device ID of AMD's, so is it appropriate > to document with the name "Hygon CZ" or just "Hygon"? "Hygon CZ" please, as Hygon is the vendor not the device, and I can imagine you may create more devices in the future. Thanks, -- Jean Delvare SUSE L3 Support
Re: [RFC PATCH RESEND] i2c-piix4: Add Hygon Dhyana SMBus support
Hi Pu,
On Mon, 22 Apr 2019 21:10:07 +0800, Pu Wen wrote:
> The Hygon Dhyana CPU has the SMBus device with PCI device ID 0x790b,
> which is the same as AMD CZ SMBus device. So add Hygon Dhyana support
> to the i2c-piix4 driver by using the code path of AMD.
Sorry for the late answer.
> Cc: # v5.0+
I object to adding this to stable. It's not fixing any bug, and it's
far from being a one-liner. I'd rather let distributions backport it as
they see fit.
> Signed-off-by: Pu Wen
> ---
> drivers/i2c/busses/i2c-piix4.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 90946a8..9db9d9d 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -289,6 +289,9 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>PIIX4_dev->revision >= 0x41) ||
> (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD &&
>PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS &&
> + PIIX4_dev->revision >= 0x49) ||
> + (PIIX4_dev->vendor == PCI_VENDOR_ID_HYGON &&
> + PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS &&
>PIIX4_dev->revision >= 0x49))
> smb_en = 0x00;
Does the compatibility with the original AMD chipset include the
revision number?
> else
> @@ -361,7 +364,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>piix4_smba, i2ccfg >> 4);
>
> /* Find which register is used for port selection */
> - if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
> + if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD ||
> + PIIX4_dev->vendor == PCI_VENDOR_ID_HYGON) {
> switch (PIIX4_dev->device) {
> case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
> piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_KERNCZ;
> @@ -794,6 +798,7 @@ static const struct pci_device_id piix4_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
>PCI_DEVICE_ID_SERVERWORKS_OSB4) },
> { PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
> @@ -904,11 +909,13 @@ static int piix4_probe(struct pci_dev *dev, const
> struct pci_device_id *id)
> if ((dev->vendor == PCI_VENDOR_ID_ATI &&
>dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>dev->revision >= 0x40) ||
> - dev->vendor == PCI_VENDOR_ID_AMD) {
> + dev->vendor == PCI_VENDOR_ID_AMD ||
> + dev->vendor == PCI_VENDOR_ID_HYGON) {
> bool notify_imc = false;
> is_sb800 = true;
>
> - if (dev->vendor == PCI_VENDOR_ID_AMD &&
> + if ((dev->vendor == PCI_VENDOR_ID_AMD ||
> + dev->vendor == PCI_VENDOR_ID_HYGON) &&
> dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
> u8 imc;
>
Patch looks good. I assume you have tested it on real hardware?
I would like you to also document the new supported chipset in
drivers/i2c/busses/Kconfig and Documentation/i2c/busses/i2c-piix4 as
well as in the header comment of i2c-piix4.c itself. I know it seems
redundant but it helps the user know which driver they need.
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 2/2] docs: hwmon: remove the extension from .rst files
On Fri, 2019-04-19 at 07:30 -0300, Mauro Carvalho Chehab wrote: > On almost all places, we're including ReST files without the > extension. > > Let's remove the extension here as well, in order to use just > one standard. > Tab in the middle of a sentence. > Suggested-by: Jani Nikula The line above appears to be truncated. > Signed-off-by: Mauro Carvalho Chehab > --- > Documentation/hwmon/index.rst | 316 +- > 1 file changed, 158 insertions(+), 158 deletions(-) -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: isch: Remove unnecessary acpi.h include
Hi Bjorn,
On Mon, 2019-04-01 at 08:50 -0500, Bjorn Helgaas wrote:
> On Mon, Mar 25, 2019 at 1:37 PM wrote:
> >
> > From: Bjorn Helgaas
> >
> > 54fb4a05af0a ("i2c: Check for ACPI resource conflicts") included
> > so we could use acpi_check_region(). fd46a0064af1 ("i2c:
> > convert i2c-isch to platform_device") removed the use of
> > acpi_check_region() but not the include.
> >
> > Remove the now-unnecessary include of . No functional change
> > intended.
> >
> > Signed-off-by: Bjorn Helgaas
>
> Added
>
> Fixes: fd46a0064af1 ("i2c: convert i2c-isch to platform_device")
> Reviewed-by: Jean Delvare
> Reviewed-by: Mukesh Ojha
>
> on my local branch.
>
> Jean, would you like me to repost this with the updates? I assume you
> will merge this (just based on get_maintainer.pl)?
No, Wolfram prefers to pick all the i2c patches into his own branch,
I'm only giving my Reviewed-by. So your question is for him.
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: isch: Remove unnecessary acpi.h include
On Mon, 25 Mar 2019 13:36:52 -0500, helg...@kernel.org wrote:
> From: Bjorn Helgaas
>
> 54fb4a05af0a ("i2c: Check for ACPI resource conflicts") included
> so we could use acpi_check_region(). fd46a0064af1 ("i2c:
> convert i2c-isch to platform_device") removed the use of
> acpi_check_region() but not the include.
>
> Remove the now-unnecessary include of . No functional change
> intended.
>
> Signed-off-by: Bjorn Helgaas
> ---
> drivers/i2c/busses/i2c-isch.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> index 5c754bf659e2..f64c1d72f73f 100644
> --- a/drivers/i2c/busses/i2c-isch.c
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -30,7 +30,6 @@
> #include
> #include
> #include
> -#include
>
> /* SCH SMBus address offsets */
> #define SMBHSTCNT(0 + sch_smba)
Reviewed-by: Jean Delvare
(I would add a Fixes tag but maybe that's just me.)
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] spi: mediatek: Attempt to address style issues in spi-mt7621.c
Hi Dan, On Thu, 14 Mar 2019 14:27:32 +0300, Dan Carpenter wrote: > On Thu, Mar 14, 2019 at 12:13:15PM +0100, Armando Miraglia wrote: > > Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl > > --fix? If one would like to contribute to fixing the tooling for linting > > which > > of the two would be the right target for such an effort? > > I've added Jean to the CC list because he worked on Lindent a few > years ago. I really think we should just delete Lindent. I haven't > used the checkpatch.pl --fix option but Joe Perches has good style. I merely fixed obvious but minor issues in Lindent as I stumbled upon them the one time in my life I used that script. That was years ago. I'm not using it on a regular basis. My principle is that if a script is present in the kernel tree then it can and should be maintained. If it is deemed not worth the maintenance effort then it should be deleted. I don't care either way. -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: sis630: correct format strings
Hi Louis,
On Sat, 2 Mar 2019 14:18:36 +, Louis Taylor wrote:
> When compiling with -Wformat, clang warns:
>
> drivers/i2c/busses/i2c-sis630.c:482:4: warning: format specifies type
> 'unsigned short' but the argument has type 'int' [-Wformat]
> smbus_base + SMB_STS,
> ^~~~
>
> drivers/i2c/busses/i2c-sis630.c:483:4: warning: format specifies type
> 'unsigned short' but the argument has type 'int' [-Wformat]
> smbus_base + SMB_STS + SIS630_SMB_IOREGION - 1);
> ^~
>
> drivers/i2c/busses/i2c-sis630.c:531:37: warning: format specifies type
> 'unsigned short' but the argument has type 'int' [-Wformat]
> "SMBus SIS630 adapter at %04hx", smbus_base + SMB_STS);
> ~ ^~~~
>
> This patch fixes the format strings to use the format type for int.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor
> ---
> drivers/i2c/busses/i2c-sis630.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sis630.c b/drivers/i2c/busses/i2c-sis630.c
> index 1e6805b5cef2..a57aa4fe51a4 100644
> --- a/drivers/i2c/busses/i2c-sis630.c
> +++ b/drivers/i2c/busses/i2c-sis630.c
> @@ -478,7 +478,7 @@ static int sis630_setup(struct pci_dev *sis630_dev)
> if (!request_region(smbus_base + SMB_STS, SIS630_SMB_IOREGION,
> sis630_driver.name)) {
> dev_err(_dev->dev,
> - "I/O Region 0x%04hx-0x%04hx for SMBus already in
> use.\n",
> + "I/O Region 0x%04x-0x%04x for SMBus already in use.\n",
> smbus_base + SMB_STS,
> smbus_base + SMB_STS + SIS630_SMB_IOREGION - 1);
> retval = -EBUSY;
> @@ -528,7 +528,7 @@ static int sis630_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> sis630_adapter.dev.parent = >dev;
>
> snprintf(sis630_adapter.name, sizeof(sis630_adapter.name),
> - "SMBus SIS630 adapter at %04hx", smbus_base + SMB_STS);
> + "SMBus SIS630 adapter at %04x", smbus_base + SMB_STS);
>
> return i2c_add_adapter(_adapter);
> }
Signed-off-by: Jean Delvare
Would be nice if gcc itself would reports such formatting issues...
Thanks,
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"
On Tue, 2018-12-11 at 13:06 +0100, Peter Korsgaard wrote: > > > > > > "Peter" == Peter Korsgaard writes: > > Hi Jean, > > >> Look, you can imagine that I was perfectly aware of what I was doing > >> when I made that change, and that I pondered the decision carefully at > >> that time. And my decision was that the change should be made. As far > >> as I'm concerned, this ship has sailed already, sorry. > > > Sorry, what is the perceived risk of reverting this change? Just the > > minor inconsistency between the dmidecode and sysfs output? As stated > > above, the RFC requires conforming parsers to handle upper case as well. > > I would appreciate if you could explain what risk you see from reverting > this change? The exact same risk that you are complaining about, for a different pair of kernel versions. You cannot at the same time argue that the change should not have been done back then, and ask for same change to be done again now. -- Jean Delvare SUSE L3 Support
Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"
On Thu, 2018-12-06 at 10:22 +0100, Peter Korsgaard wrote: > > > > > > "Jean" == Jean Delvare writes: > > > On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote: > >> This reverts commit 712ff25450bd01366301eef81c33e865d901e7b7. > >> > >> The output of dmi_save_uuid() is exposed to user space as > >> /sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards > compatibility, > >> E.G. I have systems that include the content of dmi/id/product_uuid as > part > >> of the keyphrase for cryptsetup luksOpen. > >> > >> As the change was purely cosmetical, revert it to fix such breakage. > > > The change is not "cosmetical". The change was done to comply with RFC > > 4122: > > > https://tools.ietf.org/html/rfc4122 > > > The hexadecimal values "a" through "f" are output as > > lower case characters and are case insensitive on input. > > I get that - but it changes the content of sysfs entries, breaking real > systems - E.G. a user space ABI regression. I know it's very convenient to play the "user-space ABI regression" card whenever you want a change reverted. However the interface itself did not change. The sysfs file name did not change, the nature of its contents did not change. The only thing which changed is the exact contents details of the file. In my opinion that does not qualify as an "ABI regression". > It is a cosmetic code change in the sense that no known software was > broken with the upper case characters. It bothered someone enough to create a ticket asking me to fix it in dmidecode: https://savannah.nongnu.org/bugs/index.php?53569 And it wouldn't make sense that the kernel uses a different format from dmidecode. > > If "cryptsetup luksOpen" does not lowercase digits before computing its > > key passphrase, then it's not RFC 4122-compliant and should be fixed. > > cryptsetup naturally doesn't know anything about RFC 4122. It just reads > a disk encryption keyphrase which happen to include the content of > id/product_uuid because of my scripts. OK, so basically your script infringes RFC 4122, and instead of fixing that, you ask me to stop respecting that RFC on my side. Out of curiosity, what's the purpose of your encryption strategy? That someone who would open your computer and steal only the disk (as opposed to stealing the whole computer) would be unable to read the disk's contents? Do thieves really do that? > > Nak. This is too late. Changing it again would just add confusion. > > Please reconsider. 4.17 is from June, and 4.19 has only recently become > LTS. I can't see how this is related with kernel 4.19 becoming LTS. My patch has been public since April 8th, 2018. It has been in 3 official kernel versions already. I did not hear any complaint about that change in 8 months. You are the first one to complain, and that's about a custom script that's not RFC-compliant and that you could fix with a one-liner. Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry. -- Jean Delvare SUSE L3 Support
Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"
On Thu, 2018-12-06 at 10:22 +0100, Peter Korsgaard wrote: > > > > > > "Jean" == Jean Delvare writes: > > > On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote: > >> This reverts commit 712ff25450bd01366301eef81c33e865d901e7b7. > >> > >> The output of dmi_save_uuid() is exposed to user space as > >> /sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards > compatibility, > >> E.G. I have systems that include the content of dmi/id/product_uuid as > part > >> of the keyphrase for cryptsetup luksOpen. > >> > >> As the change was purely cosmetical, revert it to fix such breakage. > > > The change is not "cosmetical". The change was done to comply with RFC > > 4122: > > > https://tools.ietf.org/html/rfc4122 > > > The hexadecimal values "a" through "f" are output as > > lower case characters and are case insensitive on input. > > I get that - but it changes the content of sysfs entries, breaking real > systems - E.G. a user space ABI regression. I know it's very convenient to play the "user-space ABI regression" card whenever you want a change reverted. However the interface itself did not change. The sysfs file name did not change, the nature of its contents did not change. The only thing which changed is the exact contents details of the file. In my opinion that does not qualify as an "ABI regression". > It is a cosmetic code change in the sense that no known software was > broken with the upper case characters. It bothered someone enough to create a ticket asking me to fix it in dmidecode: https://savannah.nongnu.org/bugs/index.php?53569 And it wouldn't make sense that the kernel uses a different format from dmidecode. > > If "cryptsetup luksOpen" does not lowercase digits before computing its > > key passphrase, then it's not RFC 4122-compliant and should be fixed. > > cryptsetup naturally doesn't know anything about RFC 4122. It just reads > a disk encryption keyphrase which happen to include the content of > id/product_uuid because of my scripts. OK, so basically your script infringes RFC 4122, and instead of fixing that, you ask me to stop respecting that RFC on my side. Out of curiosity, what's the purpose of your encryption strategy? That someone who would open your computer and steal only the disk (as opposed to stealing the whole computer) would be unable to read the disk's contents? Do thieves really do that? > > Nak. This is too late. Changing it again would just add confusion. > > Please reconsider. 4.17 is from June, and 4.19 has only recently become > LTS. I can't see how this is related with kernel 4.19 becoming LTS. My patch has been public since April 8th, 2018. It has been in 3 official kernel versions already. I did not hear any complaint about that change in 8 months. You are the first one to complain, and that's about a custom script that's not RFC-compliant and that you could fix with a one-liner. Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry. -- Jean Delvare SUSE L3 Support
Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"
On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote: > This reverts commit 712ff25450bd01366301eef81c33e865d901e7b7. > > The output of dmi_save_uuid() is exposed to user space as > /sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, > E.G. I have systems that include the content of dmi/id/product_uuid as part > of the keyphrase for cryptsetup luksOpen. > > As the change was purely cosmetical, revert it to fix such breakage. The change is not "cosmetical". The change was done to comply with RFC 4122: https://tools.ietf.org/html/rfc4122 The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input. If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed. > Signed-off-by: Peter Korsgaard > --- > drivers/firmware/dmi_scan.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 099d83e4e910..2ed51651565f 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -211,9 +211,9 @@ static void __init dmi_save_uuid(const struct dmi_header > *dm, int slot, >* says that this is the defacto standard. >*/ > if (dmi_ver >= 0x020600) > - sprintf(s, "%pUl", d); > + sprintf(s, "%pUL", d); > else > - sprintf(s, "%pUb", d); > + sprintf(s, "%pUB", d); > > dmi_ident[slot] = s; > } Nak. This is too late. Changing it again would just add confusion. -- Jean Delvare SUSE L3 Support
Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"
On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote: > This reverts commit 712ff25450bd01366301eef81c33e865d901e7b7. > > The output of dmi_save_uuid() is exposed to user space as > /sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, > E.G. I have systems that include the content of dmi/id/product_uuid as part > of the keyphrase for cryptsetup luksOpen. > > As the change was purely cosmetical, revert it to fix such breakage. The change is not "cosmetical". The change was done to comply with RFC 4122: https://tools.ietf.org/html/rfc4122 The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input. If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed. > Signed-off-by: Peter Korsgaard > --- > drivers/firmware/dmi_scan.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 099d83e4e910..2ed51651565f 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -211,9 +211,9 @@ static void __init dmi_save_uuid(const struct dmi_header > *dm, int slot, >* says that this is the defacto standard. >*/ > if (dmi_ver >= 0x020600) > - sprintf(s, "%pUl", d); > + sprintf(s, "%pUL", d); > else > - sprintf(s, "%pUb", d); > + sprintf(s, "%pUB", d); > > dmi_ident[slot] = s; > } Nak. This is too late. Changing it again would just add confusion. -- Jean Delvare SUSE L3 Support
Re: [PATCH 4.14 044/100] ACPICA: AML interpreter: add region addresses in global list during initialization
On Thu, 29 Nov 2018 20:21:37 +0100, Greg Kroah-Hartman wrote: > On Thu, Nov 29, 2018 at 06:56:40PM +, Schmauss, Erik wrote: > > This should only apply to 4.17 or later. I unintentionally put all > > applicable so > > please drop this for all 4.16 or earlier. I've learned my lesson and I'll > > put the > > correct tags from now on :-) > > Ok, now dropped from 4.14, thanks. Should be dropped from 4.9 and 4.4 too... if it was not clear. Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH 4.14 044/100] ACPICA: AML interpreter: add region addresses in global list during initialization
On Thu, 29 Nov 2018 20:21:37 +0100, Greg Kroah-Hartman wrote: > On Thu, Nov 29, 2018 at 06:56:40PM +, Schmauss, Erik wrote: > > This should only apply to 4.17 or later. I unintentionally put all > > applicable so > > please drop this for all 4.16 or earlier. I've learned my lesson and I'll > > put the > > correct tags from now on :-) > > Ok, now dropped from 4.14, thanks. Should be dropped from 4.9 and 4.4 too... if it was not clear. Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH 4.14 044/100] ACPICA: AML interpreter: add region addresses in global list during initialization
Hi Greg, On Thu, 2018-11-29 at 15:12 +0100, Greg Kroah-Hartman wrote: > 4.14-stable review patch. If anyone has any objections, please let me know. > > -- > > From: Erik Schmauss > > commit 4abb951b73ff0a8a979113ef185651aa3c8da19b upstream. > > The table load process omitted adding the operation region address > range to the global list. This omission is problematic because the OS > queries the global list to check for address range conflicts before > deciding which drivers to load. This commit may result in warning > messages that look like the following: > > [7.871761] ACPI Warning: system_IO range 0x0428-0x042F conflicts > with op_region 0x0400-0x047F (\PMIO) (20180531/utaddress-213) > [7.871769] ACPI: If an ACPI driver is available for this device, you > should use it instead of the native driver > > However, these messages do not signify regressions. It is a result of > properly adding address ranges within the global address list. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200011 > Tested-by: Jean-Marc Lenoir > Signed-off-by: Erik Schmauss > Cc: All applicable > Signed-off-by: Rafael J. Wysocki > Cc: Jean Delvare > Signed-off-by: Greg Kroah-Hartman I'm confused. While we were discussing the regression, Erik said that this is fixing commit 5a8361f7ecceaed64b4064000d16cb703462be49, which went upstream in v4.17. So how can the fix be needed in any kernel older than v4.17? Erik, did I understand you incorrectly? -- Jean Delvare SUSE L3 Support
