[PATCH] ARM: dts: omap3-igep: Change email address in copyright notice
I've switched employer a long time ago and the mentioned email address no longer exists. Use my personal address to prevent the issue in the future. Signed-off-by: Javier Martinez Canillas --- arch/arm/boot/dts/omap3-igep.dtsi| 2 +- arch/arm/boot/dts/omap3-igep0020-common.dtsi | 2 +- arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 2 +- arch/arm/boot/dts/omap3-igep0020.dts | 2 +- arch/arm/boot/dts/omap3-igep0030-common.dtsi | 2 +- arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 2 +- arch/arm/boot/dts/omap3-igep0030.dts | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/arch/arm/boot/dts/omap3-igep.dtsi b/arch/arm/boot/dts/omap3-igep.dtsi index 5de2be9bbe6..99f5585097a 100644 --- a/arch/arm/boot/dts/omap3-igep.dtsi +++ b/arch/arm/boot/dts/omap3-igep.dtsi @@ -2,7 +2,7 @@ /* * Common device tree for IGEP boards based on AM/DM37x * - * Copyright (C) 2012 Javier Martinez Canillas + * Copyright (C) 2012 Javier Martinez Canillas * Copyright (C) 2012 Enric Balletbo i Serra */ /dts-v1/; diff --git a/arch/arm/boot/dts/omap3-igep0020-common.dtsi b/arch/arm/boot/dts/omap3-igep0020-common.dtsi index af8aa5f0feb..73d8f471b9e 100644 --- a/arch/arm/boot/dts/omap3-igep0020-common.dtsi +++ b/arch/arm/boot/dts/omap3-igep0020-common.dtsi @@ -2,7 +2,7 @@ /* * Common Device Tree Source for IGEPv2 * - * Copyright (C) 2014 Javier Martinez Canillas + * Copyright (C) 2014 Javier Martinez Canillas * Copyright (C) 2014 Enric Balletbo i Serra */ diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts index 567232584f0..9dca5bfc87a 100644 --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts @@ -2,7 +2,7 @@ /* * Device Tree Source for IGEPv2 Rev. F (TI OMAP AM/DM37x) * - * Copyright (C) 2012 Javier Martinez Canillas + * Copyright (C) Javier Martinez Canillas * Copyright (C) 2012 Enric Balletbo i Serra */ diff --git a/arch/arm/boot/dts/omap3-igep0020.dts b/arch/arm/boot/dts/omap3-igep0020.dts index e341535a716..c6f863bc03a 100644 --- a/arch/arm/boot/dts/omap3-igep0020.dts +++ b/arch/arm/boot/dts/omap3-igep0020.dts @@ -2,7 +2,7 @@ /* * Device Tree Source for IGEPv2 Rev. C (TI OMAP AM/DM37x) * - * Copyright (C) 2012 Javier Martinez Canillas + * Copyright (C) 2012 Javier Martinez Canillas * Copyright (C) 2012 Enric Balletbo i Serra */ diff --git a/arch/arm/boot/dts/omap3-igep0030-common.dtsi b/arch/arm/boot/dts/omap3-igep0030-common.dtsi index 71b0ae807ec..742e3e14706 100644 --- a/arch/arm/boot/dts/omap3-igep0030-common.dtsi +++ b/arch/arm/boot/dts/omap3-igep0030-common.dtsi @@ -2,7 +2,7 @@ /* * Common Device Tree Source for IGEP COM MODULE * - * Copyright (C) 2014 Javier Martinez Canillas + * Copyright (C) 2014 Javier Martinez Canillas * Copyright (C) 2014 Enric Balletbo i Serra */ diff --git a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts index df6ba121983..8e9c12cf51a 100644 --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts @@ -2,7 +2,7 @@ /* * Device Tree Source for IGEP COM MODULE Rev. G (TI OMAP AM/DM37x) * - * Copyright (C) 2014 Javier Martinez Canillas + * Copyright (C) 2014 Javier Martinez Canillas * Copyright (C) 2014 Enric Balletbo i Serra */ diff --git a/arch/arm/boot/dts/omap3-igep0030.dts b/arch/arm/boot/dts/omap3-igep0030.dts index 32f31035daa..5188f96f431 100644 --- a/arch/arm/boot/dts/omap3-igep0030.dts +++ b/arch/arm/boot/dts/omap3-igep0030.dts @@ -2,7 +2,7 @@ /* * Device Tree Source for IGEP COM MODULE Rev. E (TI OMAP AM/DM37x) * - * Copyright (C) 2012 Javier Martinez Canillas + * Copyright (C) 2012 Javier Martinez Canillas * Copyright (C) 2012 Enric Balletbo i Serra */ -- 2.29.2
Re: [PATCH RFC] modpost: Support I2C Aliases from OF tables
Hello Geert, On 8/6/19 9:30 AM, Geert Uytterhoeven wrote: > On Tue, Aug 6, 2019 at 12:48 AM Javier Martinez Canillas > wrote: >> On 8/1/19 4:17 AM, Masahiro Yamada wrote: >> So I think that we should either: >> >> a) take Kieran's patch or b) remove the i2c_of_match_device_sysfs() fallback >> for OF and require an I2C device table for sysfs instantiation and matching. >> >>> If a driver supports DT and devices are instantiated via DT, >>> in which situation is this useful? >> >> Is useful if you don't have all the I2C devices described in the DT. For >> example >> a daughterboard with an I2C device is connected to a board through an >> expansion >> slot or an I2C device connected directly to I2C pins exposed in a machine. >> >> In these cases your I2C devices won't be static so users might want to use >> the >> sysfs user-space interface to instantiate the I2C devices, i.e: >> >> # echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-3/new_device >> >> as explained in >> https://github.com/torvalds/linux/blob/master/Documentation/i2c/instantiating-devices#L207 > > Does this actually work with DT names, too? E.g. > > # echo atmel,24c02 > /sys/bus/i2c/devices/i2c-3/new_device > My understanding is that it does. If I'm reading the code correctly the i2c_of_match_device_sysfs() function first attempts to match using both the vendor and device part of the compatible string and if that fails, it strips the vendor part and try to match only using the device part. So you could use any of the following: # echo 24c02 0x50 > /sys/bus/i2c/devices/i2c-3/new_device # echo atmel,24c02 0x50 > /sys/bus/i2c/devices/i2c-3/new_device Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH RFC] modpost: Support I2C Aliases from OF tables
Hello Geert,
On 8/6/19 9:22 AM, Geert Uytterhoeven wrote:
> Hi Javier,
>
> On Tue, Aug 6, 2019 at 12:25 AM Javier Martinez Canillas
> wrote:
>> On 7/31/19 9:44 PM, Wolfram Sang wrote:
>>> Hi Javier,
>>>> The other option is to remove i2c_of_match_device() and don't make OF match
>>>> to fallback to i2c_of_match_device_sysfs(). This is what happens in the
>>>> ACPI
>>>> case, since i2c_device_match() just calls acpi_driver_match_device()
>>>> directly
>>>> and doesn't have a wrapper function that fallbacks to sysfs matching.
>>>>
>>>> In this case an I2C device ID table would be required if the devices have
>>>> to
>>>> be instantiated through sysfs. That way the I2C table would be used both
>>>> for
>>>> auto-loading and also to match the device when it doesn't have an of_node.
>>>
>>> That would probably mean that only a minority of drivers will not add an I2C
>>> device ID table because it is easy to add an you get the sysfs feature?
>>>
>>
>> I believe so yes.
>
>> As Masahiro-san mentioned, this approach will still require to add a new
>> macro
>> MODULE_DEVICE_TABLE(i2c_of, bar_of_match) so the OF device table is used
>> twice.
>>
>> One to expose the "of:N*T*Cfoo,bar" and another one to expose it as
>> "i2c:bar".
>>
>> I expect that many developers would miss adding this macro for new drivers
>> that
>> are DT-only and so sysfs instantiation would not work there. So whatever is
>> the
>> approach taken we should clearly document all this so drivers authors are
>> aware.
>
> You could add a new I2C_MODULE_DEVICE_TABLE() that adds both, right?
> Makes it a little bit easier to check/enforce this.
>
Right, we could add a macro for that. Although it should probably be called
I2C_OF_MODULE_DEVICE_TABLE() or something like that since is specific to OF.
> Gr{oetje,eeting}s,
>
> Geert
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH RFC] modpost: Support I2C Aliases from OF tables
Hello Masahiro-san, On 8/1/19 4:17 AM, Masahiro Yamada wrote: > Hi. > > On Thu, Aug 1, 2019 at 4:44 AM Wolfram Sang wrote: >> >> Hi Javier, >> >> thank you for providing the extra information. >> >> (And Kieran, thanks for the patch!) >> >>> The other option is to remove i2c_of_match_device() and don't make OF match >>> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI >>> case, since i2c_device_match() just calls acpi_driver_match_device() >>> directly >>> and doesn't have a wrapper function that fallbacks to sysfs matching. >>> >>> In this case an I2C device ID table would be required if the devices have to >>> be instantiated through sysfs. That way the I2C table would be used both for >>> auto-loading and also to match the device when it doesn't have an of_node. >> >> That would probably mean that only a minority of drivers will not add an I2C >> device ID table because it is easy to add an you get the sysfs feature? >> >> Then we are back again with the situation that most drivers will have >> multiple tables. With the minor change that the I2C device id table is >> not required anymore by the core, but it will be just very useful to >> have? Or? >> >>> If the former is the correct way to solve this then the patch looks good to >>> me. >>> >>> Reviewed-by: Javier Martinez Canillas >> >> For this actual patch from Kieran, I'd like to hear an opinion from the >> people maintaining modpost. > > > As you see 'git log scripts/mod/file2alias.c', > this file is touched by every subsystem. > > So, the decision is up to you, Wolfram. > And, you can pick this to your tree if you like. > > > The implementation is really trivial. > > > As Javier pointed out, this discussion comes down to > "do we want to fall back to i2c_of_match_device_sysfs()?" > Yes, I think that's the crux of the matter. Basically the matching logic should be consistent with the modalias uevent exposed to user-space to auto-load modules. So I think that we should either: a) take Kieran's patch or b) remove the i2c_of_match_device_sysfs() fallback for OF and require an I2C device table for sysfs instantiation and matching. > If a driver supports DT and devices are instantiated via DT, > in which situation is this useful? Is useful if you don't have all the I2C devices described in the DT. For example a daughterboard with an I2C device is connected to a board through an expansion slot or an I2C device connected directly to I2C pins exposed in a machine. In these cases your I2C devices won't be static so users might want to use the sysfs user-space interface to instantiate the I2C devices, i.e: # echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-3/new_device as explained in https://github.com/torvalds/linux/blob/master/Documentation/i2c/instantiating-devices#L207 > Do legacy non-DT platforms need this? Yes, can also be used by non-DT platforms. But in this case isn't a problem because drivers for these platform will already have an I2C device ID table. > > > >> The aproach looks okay to me, yet I can't >> tell how "easy" we are with adding new types like 'i2c_of'. > > As far as I understood, this patch provides a shorthand. > You can save one table, but still get the > same MODULE_ALIAS in the *.mod.c file. > You need to add two MODULE_DEVICE_TABLE() though. > > MODULE_DEVICE_TABLE(of, si4713_of_match); > MODULE_DEVICE_TABLE(i2c_of, si4713_of_match); > That's my understanding as well. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH RFC] modpost: Support I2C Aliases from OF tables
Hello Wolfram, On 7/31/19 9:44 PM, Wolfram Sang wrote: > Hi Javier, > > thank you for providing the extra information. > > (And Kieran, thanks for the patch!) > >> The other option is to remove i2c_of_match_device() and don't make OF match >> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI >> case, since i2c_device_match() just calls acpi_driver_match_device() directly >> and doesn't have a wrapper function that fallbacks to sysfs matching. >> >> In this case an I2C device ID table would be required if the devices have to >> be instantiated through sysfs. That way the I2C table would be used both for >> auto-loading and also to match the device when it doesn't have an of_node. > > That would probably mean that only a minority of drivers will not add an I2C > device ID table because it is easy to add an you get the sysfs feature? > I believe so yes. > Then we are back again with the situation that most drivers will have > multiple tables. With the minor change that the I2C device id table is > not required anymore by the core, but it will be just very useful to > have? Or? > Yes, it won't be needed anymore if you are only instantiating all your devices from your firmware interface (e.g: OF, ACPI). >> If the former is the correct way to solve this then the patch looks good to >> me. >> >> Reviewed-by: Javier Martinez Canillas > > For this actual patch from Kieran, I'd like to hear an opinion from the > people maintaining modpost. The aproach looks okay to me, yet I can't > tell how "easy" we are with adding new types like 'i2c_of'. > As Masahiro-san mentioned, this approach will still require to add a new macro MODULE_DEVICE_TABLE(i2c_of, bar_of_match) so the OF device table is used twice. One to expose the "of:N*T*Cfoo,bar" and another one to expose it as "i2c:bar". I expect that many developers would miss adding this macro for new drivers that are DT-only and so sysfs instantiation would not work there. So whatever is the approach taken we should clearly document all this so drivers authors are aware. > Thanks everyone, > >Wolfram > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH RFC] modpost: Support I2C Aliases from OF tables
Hello Kieran, On 7/10/19 9:39 PM, Kieran Bingham wrote: > I2C drivers match against an I2C ID table, an OF table, and an ACPI > table. It is now also possible to match against an OF table entry > without the vendor prefix to support backwards compatibility, and allow > simplification of the i2c probe functions. > > As part of this matching, the probe function is being converted to > remove the need to specify the i2c_device_id table, but to support > module aliasing, we still require to have the MODULE_DEVICE_TABLE entry. > My opinion on this is that I2C drivers should register the tables of the firmware interfaces that support. That is, if a driver is only used in a platform that supports OF then it should only require an OF device table. But if the driver supports devices that can also be present in platforms that use ACPI, then should also require to have an ACPI device ID table. So there should be consistency about what table is used for both matching a device with a driver and reporting a modalias to user-space for module auto-loading. If a I2C device was instantiated by OF, then the OF table should be used for both reporting a modalias uevent and matching a driver. Now, the i2c_of_match_device() function attempts to match by first calling of_match_device() and if fails fallbacks to i2c_of_match_device_sysfs(). The latter attempts to match the I2C device by striping the vendor prefix of the compatible strings on the OF device ID table. That means that you could instantiate an I2C device ID through the sysfs interface and the OF table would be used to match the driver. But i2c_device_uevent() would had reported an I2C modalias and not an OF modalias (since the registered device won't have an associated of_node) so there's inconsistency in that case since a table is used to match but no to report modaliases. > Facilitate generating the I2C aliases directly from the of_device_id > tables, by stripping the vendor prefix prefix from the compatible string > and using that as an alias just as the i2c-core supports. > I see two ways to solve this issue. One is with $SUBJECT since we can argue that if a OF-only driver is able to match devices that were instantiated through the sysfs interface that only have a device name, then a modalias of the form i2c: is needed. Since the compatible strings without the vendor prefix is used to match, then I think that makes sense to also use them without the vendor prefix to populate I2C modaliases as $SUBJECT does. The other option is to remove i2c_of_match_device() and don't make OF match to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI case, since i2c_device_match() just calls acpi_driver_match_device() directly and doesn't have a wrapper function that fallbacks to sysfs matching. In this case an I2C device ID table would be required if the devices have to be instantiated through sysfs. That way the I2C table would be used both for auto-loading and also to match the device when it doesn't have an of_node. If the former is the correct way to solve this then the patch looks good to me. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
[PATCH] media: staging/imx: Allow driver to build if COMPILE_TEST is enabled
The driver has runtime but no build time dependency with IMX_IPUV3_CORE, so can be built for testing purposes if COMPILE_TEST option is enabled. This is useful to have more build coverage and make sure that the driver is not affected by changes that could cause build regressions. Signed-off-by: Javier Martinez Canillas --- drivers/staging/media/imx/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig index 36b276ea2ec..5045e24c470 100644 --- a/drivers/staging/media/imx/Kconfig +++ b/drivers/staging/media/imx/Kconfig @@ -1,7 +1,7 @@ config VIDEO_IMX_MEDIA tristate "i.MX5/6 V4L2 media core driver" depends on ARCH_MXC || COMPILE_TEST - depends on MEDIA_CONTROLLER && VIDEO_V4L2 && IMX_IPUV3_CORE + depends on MEDIA_CONTROLLER && VIDEO_V4L2 && (IMX_IPUV3_CORE || COMPILE_TEST) depends on VIDEO_V4L2_SUBDEV_API depends on HAS_DMA select VIDEOBUF2_DMA_CONTIG -- 2.20.1
Re: [PATCH 3/3] drivers: use probe_err function in obvious cases
Hello Andrzej, On 10/16/2018 09:22 AM, Andrzej Hajda wrote: > The patch replaces obviously matching code with probe_err function. > There are many more places where probe_err could be used. > The patch shows how the new function should be used, and how it > improves the code. > > It was generated by cocci script (little bit dirty): > This is great! I guess the easier would be to split these by kernel releases, maybe patch #1 and #2 can land in one kernel release and then each subsystem can merge the changes for their drivers with the dependency already in place. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 2/3] driver core: add deferring probe reason to devices_deferred property
On 10/16/2018 09:22 AM, Andrzej Hajda wrote:
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
> message
> will be attached to deferred device and printed when user read
> devices_deferred
> property.
>
> Signed-off-by: Andrzej Hajda
> ---
> drivers/base/base.h | 3 +++
> drivers/base/core.c | 15 +--
> drivers/base/dd.c | 34 +-
> 3 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..6f9371bfe40b 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
> struct klist_node knode_driver;
> struct klist_node knode_bus;
> struct list_head deferred_probe;
> + char *deferred_probe_msg;
> struct device *device;
> };
> #define to_device_private_parent(obj)\
> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device
> *dev,
> extern void driver_detach(struct device_driver *drv);
> extern int driver_probe_device(struct device_driver *drv, struct device
> *dev);
> extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev, const char
> *fmt,
> + va_list args);
> static inline int driver_match_device(struct device_driver *drv,
> struct device *dev)
> {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 23fabefb217a..df9895adf11b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3076,6 +3076,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
> *
> * This helper implements common pattern present in probe functions for error
> * checking: print message if the error is not -EPROBE_DEFER and propagate
> it.
> + * In case of -EPROBE_DEFER it sets defer probe reason.
> * It replaces code sequence:
> * if (err != -EPROBE_DEFER)
> * dev_err(dev, ...);
> @@ -3091,15 +3092,17 @@ int probe_err(const struct device *dev, int err,
> const char *fmt, ...)
> struct va_format vaf;
> va_list args;
>
> - if (err != -EPROBE_DEFER) {
> - va_start(args, fmt);
> + va_start(args, fmt);
>
> - vaf.fmt = fmt;
> - vaf.va = &args;
> + vaf.fmt = fmt;
> + vaf.va = &args;
>
> + if (err != -EPROBE_DEFER)
> __dev_printk(KERN_ERR, dev, &vaf);
> - va_end(args);
> - }
> + else
> + __deferred_probe_set_msg(dev, fmt, args);
> +
> + va_end(args);
>
> return err;
> }
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..e2f81e538d4b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "base.h"
> #include "power/power.h"
> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
> if (!list_empty(&dev->p->deferred_probe)) {
> dev_dbg(dev, "Removed from deferred list\n");
> list_del_init(&dev->p->deferred_probe);
> + kfree(dev->p->deferred_probe_msg);
> + dev->p->deferred_probe_msg = NULL;
> }
> mutex_unlock(&deferred_probe_mutex);
> }
> @@ -202,6 +205,32 @@ void device_unblock_probing(void)
> driver_deferred_probe_trigger();
> }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, const char *fmt,
> + va_list args)
> +{
> + const int size = 128;
> + char **p;
> + int n;
> +
> + mutex_lock(&deferred_probe_mutex);
> +
> + p = &dev->p->deferred_probe_msg;
> + if (!*p) {
> + *p = kmalloc(size, GFP_KERNEL);
> + if (!*p)
> + goto end;
> + }
> + n = snprintf(*p, size, "%s %s: ", dev_driver_string(dev),
> dev_name(dev));
> + if (n < size)
> + vsnprintf(*p + n, size - n, fmt, args);
I wonder if the vsnprintf() return value should be checked and print a warning
if the message was truncated. Probably 128 is enough for most error messages?
Reviewed-by: Javier Martinez Canillas
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH 1/3] driver core: add probe_err log helper
Hello Andrzej, On 10/16/2018 09:22 AM, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for error > printk some message if it is not -EPROBE_DEFER and return the error. This > pattern is simple but requires adding few lines after any resource acquisition > code, as a result it is often omited or implemented only partially. > probe_err helps to replace such code seqences with simple call, so code: > if (err != -EPROBE_DEFER) > dev_err(dev, ...); > return err; > becomes: > return probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda > --- Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/2] media: intel-ipu3: allow the media graph to be used even if a subdev fails
Hi Tianshu and Sakari, On 9/4/18 1:30 PM, Javier Martinez Canillas wrote: > Hello, > > This series allows the ipu3-cio2 driver to properly expose a subset of the > media graph even if some drivers for the pending subdevices fail to probe. > > Currently the driver exposes a non-functional graph since the pad links are > created and the subdev dev nodes are registered in the v4l2 async .complete > callback. Instead, these operations should be done in the .bound callback. > > Patch #1 just adds a v4l2_device_register_subdev_node() function to allow > registering a single device node for a subdev of a v4l2 device. > > Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound > callback. The .complete callback is just removed since is empy after that. > > Best regards, > Javier > > > Javier Martinez Canillas (2): > [media] v4l: allow to register dev nodes for individual v4l2 subdevs > media: intel-ipu3: create pad links and register subdev nodes at bound > time > Any comments about these patches? Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
[PATCH 0/2] media: intel-ipu3: allow the media graph to be used even if a subdev fails
Hello, This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe. Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback. Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device. Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that. Best regards, Javier Javier Martinez Canillas (2): [media] v4l: allow to register dev nodes for individual v4l2 subdevs media: intel-ipu3: create pad links and register subdev nodes at bound time drivers/media/pci/intel/ipu3/ipu3-cio2.c | 66 ++--- drivers/media/v4l2-core/v4l2-device.c| 90 ++-- include/media/v4l2-device.h | 10 +++ 3 files changed, 85 insertions(+), 81 deletions(-) -- 2.17.1
[PATCH] tpm: suppress transmit cmd error logs when TPM 1.2 is disabled/deactivated
For TPM 1.2 chips the system setup utility allows to set the TPM device in
one of the following states:
* Active: Security chip is functional
* Inactive: Security chip is visible, but is not functional
* Disabled: Security chip is hidden and is not functional
When choosing the "Inactive" state, the TPM 1.2 device is enumerated and
registered, but sending TPM commands fail with either TPM_DEACTIVATED or
TPM_DISABLED depending if the firmware deactivated or disabled the TPM.
Since these TPM 1.2 error codes don't have special treatment, inactivating
the TPM leads to a very noisy kernel log buffer that shows messages like
the following:
tpm_tis 00:05: 1.2 TPM (device-id 0x0, rev-id 78)
tpm tpm0: A TPM error (6) occurred attempting to read a pcr value
tpm tpm0: TPM is disabled/deactivated (0x6)
tpm tpm0: A TPM error (6) occurred attempting get random
tpm tpm0: A TPM error (6) occurred attempting to read a pcr value
ima: No TPM chip found, activating TPM-bypass! (rc=6)
tpm tpm0: A TPM error (6) occurred attempting get random
tpm tpm0: A TPM error (6) occurred attempting get random
tpm tpm0: A TPM error (6) occurred attempting get random
tpm tpm0: A TPM error (6) occurred attempting get random
Let's just suppress error log messages for the TPM_{DEACTIVATED,DISABLED}
return codes, since this is expected when the TPM 1.2 is set to Inactive.
In that case the kernel log is cleaner and less confusing for users, i.e:
tpm_tis 00:05: 1.2 TPM (device-id 0x0, rev-id 78)
tpm tpm0: TPM is disabled/deactivated (0x6)
ima: No TPM chip found, activating TPM-bypass! (rc=6)
Reported-by: Hans de Goede
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm-interface.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf98..9f61106502a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -663,7 +663,8 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct
tpm_space *space,
return len;
err = be32_to_cpu(header->return_code);
- if (err != 0 && desc)
+ if (err != 0 && err != TPM_ERR_DISABLED && err != TPM_ERR_DEACTIVATED
+ && desc)
dev_err(&chip->dev, "A TPM error (%d) occurred %s\n", err,
desc);
if (err)
--
2.17.1
Re: [PATCH] mfd: sec: Export OF module alias table
Hi Krzysztof,
On Wed, Jul 25, 2018 at 5:53 PM, Krzysztof Kozlowski wrote:
> In case of Device Tree platforms, even though the Samsung PMIC sec
> device is instantiated from DT, the driver is still matched through I2C
> module alias. That is because I2C core always reports an I2C module
> alias instead of DT one.
>
Just a heads up that this already changed in v4.17 since commit
af503716ac1 ("i2c: core: report OF style module alias for devices
registered via OF").
> This could change in the future so export DT module alias.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> drivers/mfd/sec-core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> index ca6b80d08ffc..9613b4257302 100644
> --- a/drivers/mfd/sec-core.c
> +++ b/drivers/mfd/sec-core.c
> @@ -146,6 +146,7 @@ static const struct of_device_id sec_dt_match[] = {
> /* Sentinel */
> },
> };
> +MODULE_DEVICE_TABLE(of, sec_dt_match);
This driver can't be built as a module since its config symbol is
bool, so technically this macro isn't needed. But it's a no-op when
the driver is built-in so it's harmless to have it.
Best regards,
Javier
[PATCH v4] driver core: add a debugfs entry to show deferred devices
With Device Trees (DT), the dependencies of the devices are defined in the
DT, then the drivers parse that information to lookup the needed resources
that have as dependencies.
Since drivers and devices are registered in a non-deterministic way, it is
possible that a device that is a dependency has not been registered yet by
the time that is looked up.
In this case the driver that requires this dependency cannot probe and has
to defer it. So the driver core adds it to a list of deferred devices that
is iterated again every time that a new driver is probed successfully.
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/devices_deferred
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
This information could be obtained partially by enabling debugging, but it
means that the kernel log has to be parsed and the probe deferral balanced
with the successes. This can be error probe and has to be done in a ad-hoc
manner by everyone who needs to debug these kind of issues.
Since the information is already known by the kernel, just show it to make
it easier to debug.
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Mark Brown
---
Changes since RFC v1:
- Remove unneeded ret variable from deferred_devs_show()
Changes since RFC v2:
- Use DEFINE_SHOW_ATTRIBUTE() macro.
- Don't propagate debugfs_create_file() error.
- Remove IS_ENABLED(CONFIG_DEBUG_FS) guards.
- Drop RFC prefix.
Changes since v1:
- Better explain in the commit message why this patch is useful.
- Rename deferred_devices entry to devices_deferred.
- Add an exit function and remove the debugfs entry.
Changes since v2:
- Add Andy Shevchenko and Mark Brown Reviewed-by tag.
- Rebase on top of Greg's driver-core-next branch.
Changes since v3:
- Fixed a build warning caused by a wrong conflict resolution on v3.
drivers/base/dd.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6ea9c5cece7..e85705e8440 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -16,6 +16,7 @@
* Copyright (c) 2007-2009 Novell Inc.
*/
+#include
#include
#include
#include
@@ -53,6 +54,7 @@ static DEFINE_MUTEX(deferred_probe_mutex);
static LIST_HEAD(deferred_probe_pending_list);
static LIST_HEAD(deferred_probe_active_list);
static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static struct dentry *deferred_devices;
/*
* In some cases, like suspend to RAM or hibernation, It might be reasonable
@@ -199,6 +201,24 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(deferred_devs);
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -208,6 +228,9 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ deferred_devices = debugfs_create_file("devices_deferred", 0444, NULL,
+ NULL, &deferred_devs_fops);
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
@@ -216,6 +239,12 @@ static int deferred_probe_initcall(void)
}
late_initcall(deferred_probe_initcall);
+static void __exit deferred_probe_exit(void)
+{
+ debugfs_remove_recursive(deferred_devices);
+}
+__exitcall(deferred_probe_exit);
+
/**
* device_is_bound() - Check if device is bound to a driver
* @dev: device to check
--
2.17.1
Re: [PATCH v3] driver core: add a debugfs entry to show deferred devices
On 07/08/2018 03:22 PM, Greg Kroah-Hartman wrote:
> On Sun, Jul 08, 2018 at 03:21:07PM +0200, Greg Kroah-Hartman wrote:
>> On Sun, Jul 08, 2018 at 02:32:56AM +0200, Javier Martinez Canillas wrote:
>>> With Device Trees (DT), the dependencies of the devices are defined in the
>>> DT, then the drivers parse that information to lookup the needed resources
>>> that have as dependencies.
>>>
>>> Since drivers and devices are registered in a non-deterministic way, it is
>>> possible that a device that is a dependency has not been registered yet by
>>> the time that is looked up.
>>>
>>> In this case the driver that requires this dependency cannot probe and has
>>> to defer it. So the driver core adds it to a list of deferred devices that
>>> is iterated again every time that a new driver is probed successfully.
>>>
>>> For debugging purposes it may be useful to know what are the devices whose
>>> probe function was deferred. Add a debugfs entry showing that information.
>>>
>>> $ cat /sys/kernel/debug/devices_deferred
>>> 4807.i2c:twl@48:bci
>>> musb-hdrc.0.auto
>>> omapdrm.0
>>>
>>> This information could be obtained partially by enabling debugging, but it
>>> means that the kernel log has to be parsed and the probe deferral balanced
>>> with the successes. This can be error probe and has to be done in a ad-hoc
>>> manner by everyone who needs to debug these kind of issues.
>>>
>>> Since the information is already known by the kernel, just show it to make
>>> it easier to debug.
>>>
>>> Signed-off-by: Javier Martinez Canillas
>>> Reviewed-by: Andy Shevchenko
>>> Reviewed-by: Mark Brown
>>>
>>> ---
>>>
>>> Changes since RFC v1:
>>> - Remove unneeded ret variable from deferred_devs_show()
>>>
>>> Changes since RFC v2:
>>> - Use DEFINE_SHOW_ATTRIBUTE() macro.
>>> - Don't propagate debugfs_create_file() error.
>>> - Remove IS_ENABLED(CONFIG_DEBUG_FS) guards.
>>> - Drop RFC prefix.
>>>
>>> Changes since v1:
>>> - Better explain in the commit message why this patch is useful.
>>> - Rename deferred_devices entry to devices_deferred.
>>> - Add an exit function and remove the debugfs entry.
>>>
>>> Changes since v2:
>>> - Add Andy Shevchenko and Mark Brown Reviewed-by tag.
>>> - Rebase on top of Greg's driver-core-next branch.
>>>
>>>
>>> drivers/base/dd.c | 30 ++
>>> 1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>> index 6ea9c5cece7..140f534ee9c 100644
>>> --- a/drivers/base/dd.c
>>> +++ b/drivers/base/dd.c
>>> @@ -16,6 +16,7 @@
>>> * Copyright (c) 2007-2009 Novell Inc.
>>> */
>>>
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -53,6 +54,8 @@ static DEFINE_MUTEX(deferred_probe_mutex);
>>> static LIST_HEAD(deferred_probe_pending_list);
>>> static LIST_HEAD(deferred_probe_active_list);
>>> static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
>>> +static struct dentry *deferred_devices;
>>> +static bool initcalls_done;
>>
>> Wait, why add this variable? No one uses it that I can see in this
>> patch, doesn't it create a build warning?
>>
Argh, sorry about that. It was my bad when doing the conflict resolution. This
variable got removed by commit 0a50f61c4fb ("drivers: base: initcall_debug logs
for driver probe times") that made the patch to not apply cleanly anymore.
>> You did test-build this, right?
>
> I just tested it, it causes a build warning, which implies that you did
> not test this :(
>
I did but missed the build warning.
> greg k-h
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
[PATCH v3] driver core: add a debugfs entry to show deferred devices
With Device Trees (DT), the dependencies of the devices are defined in the
DT, then the drivers parse that information to lookup the needed resources
that have as dependencies.
Since drivers and devices are registered in a non-deterministic way, it is
possible that a device that is a dependency has not been registered yet by
the time that is looked up.
In this case the driver that requires this dependency cannot probe and has
to defer it. So the driver core adds it to a list of deferred devices that
is iterated again every time that a new driver is probed successfully.
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/devices_deferred
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
This information could be obtained partially by enabling debugging, but it
means that the kernel log has to be parsed and the probe deferral balanced
with the successes. This can be error probe and has to be done in a ad-hoc
manner by everyone who needs to debug these kind of issues.
Since the information is already known by the kernel, just show it to make
it easier to debug.
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Mark Brown
---
Changes since RFC v1:
- Remove unneeded ret variable from deferred_devs_show()
Changes since RFC v2:
- Use DEFINE_SHOW_ATTRIBUTE() macro.
- Don't propagate debugfs_create_file() error.
- Remove IS_ENABLED(CONFIG_DEBUG_FS) guards.
- Drop RFC prefix.
Changes since v1:
- Better explain in the commit message why this patch is useful.
- Rename deferred_devices entry to devices_deferred.
- Add an exit function and remove the debugfs entry.
Changes since v2:
- Add Andy Shevchenko and Mark Brown Reviewed-by tag.
- Rebase on top of Greg's driver-core-next branch.
drivers/base/dd.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6ea9c5cece7..140f534ee9c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -16,6 +16,7 @@
* Copyright (c) 2007-2009 Novell Inc.
*/
+#include
#include
#include
#include
@@ -53,6 +54,8 @@ static DEFINE_MUTEX(deferred_probe_mutex);
static LIST_HEAD(deferred_probe_pending_list);
static LIST_HEAD(deferred_probe_active_list);
static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static struct dentry *deferred_devices;
+static bool initcalls_done;
/*
* In some cases, like suspend to RAM or hibernation, It might be reasonable
@@ -199,6 +202,24 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(deferred_devs);
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -208,6 +229,9 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ deferred_devices = debugfs_create_file("devices_deferred", 0444, NULL,
+ NULL, &deferred_devs_fops);
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
@@ -216,6 +240,12 @@ static int deferred_probe_initcall(void)
}
late_initcall(deferred_probe_initcall);
+static void __exit deferred_probe_exit(void)
+{
+ debugfs_remove_recursive(deferred_devices);
+}
+__exitcall(deferred_probe_exit);
+
/**
* device_is_bound() - Check if device is bound to a driver
* @dev: device to check
--
2.17.1
Re: [PATCH v2] driver core: add a debugfs entry to show deferred devices
Hi Greg, On 07/07/2018 05:59 PM, Greg Kroah-Hartman wrote: > On Thu, Jun 28, 2018 at 12:06:56AM +0200, Javier Martinez Canillas wrote: >> With Device Trees (DT), the dependencies of the devices are defined in the >> DT, then the drivers parse that information to lookup the needed resources >> that have as dependencies. >> >> Since drivers and devices are registered in a non-deterministic way, it is >> possible that a device that is a dependency has not been registered yet by >> the time that is looked up. >> >> In this case the driver that requires this dependency cannot probe and has >> to defer it. So the driver core adds it to a list of deferred devices that >> is iterated again every time that a new driver is probed successfully. >> >> For debugging purposes it may be useful to know what are the devices whose >> probe function was deferred. Add a debugfs entry showing that information. >> >> $ cat /sys/kernel/debug/devices_deferred >> 4807.i2c:twl@48:bci >> musb-hdrc.0.auto >> omapdrm.0 >> >> This information could be obtained partially by enabling debugging, but it >> means that the kernel log has to be parsed and the probe deferral balanced >> with the successes. This can be error probe and has to be done in a ad-hoc >> manner by everyone who needs to debug these kind of issues. >> >> Since the information is already known by the kernel, just show it to make >> it easier to debug. >> >> Signed-off-by: Javier Martinez Canillas >> Reviewed-by: Mark Brown > > This doesn't apply to my tree anymore :( > I see, I made sure that it applied on top of linux-next. > Can you rebase and resend? > I guess you want me to rebase on top of your driver-core-next branch. I think that linux-next should pull that branch instead of the driver-core-linus one: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Next/Trees#n29 > thanks, > > greg k-h > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
Hi Andy, On 07/04/2018 06:11 PM, Andy Shevchenko wrote: > On Wed, Jul 4, 2018 at 4:44 PM, Javier Martinez Canillas > wrote: >> On 07/04/2018 03:24 PM, Nikolaus Voss wrote: > >>> I hope you're still not annoyed... >> Don't worry for that, it's very hard to get my annoyed :) > > Javier, thanks for your patience and nice explanation! > You are welcome. I'm glad you found it useful! Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
On 07/04/2018 03:24 PM, Nikolaus Voss wrote: > On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >> On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss >> wrote: >>> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >>>> >>>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss >>>> wrote: >>>> >>> >>> [snip] >>> >>>> But this discussion isn't really related to your patch. I think is >>>> correct but just said that (b) wasn't a justification to leave the I2C >>>> table, points (a) and (c) are though. I won't really be convinced that >>>> the fallback is the correct thing to do or even a good idea. >>> >>> >>> I didn't want to annoy you, I just wanted to understand why you think >>> fallback is such a bad thing that you call it a bug. And I see, it has its >>> drawbacks ;-). Anyway, thanks for taking the time to clarify this, >>> >> >> Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say >> is that I've a strong opinion on this and won't be convinced otherwise >> :) >> >> So for me is a bug because that would mean that either an entry is >> missing in an OF device table or a DTS has a node with a compatible >> string without a vendor prefix. > > Yes, I see your point (and your strong opinion :-)), but AFAIK vendor > prefix is not mandatory... At least for vendor-agnostic drivers like The latest Device Tree specification [0] says about the compatible string: "The recommended format is 'manufacturer,model', where manufacturer is a string describing the name of the manufacturer (such as a stock ticker symbol), and model specifies the model number". > "regulator-fixed" (very popular in dts files). My point is not bloating I don't think the "regulator-fixed" is a good example. Since the Device Tree should describe the hardware. The "regulator-fixed" is a convenient way to describe a fixed voltage regulator but I think is more of an exception. I'm pretty sure that the DT maintainers wouldn't ack a DT binding with a compatible string that doesn't have a manufacture prefix nowadays. > drivers with large redundant (from a driver-functional view) tables when > one table could be enough for a properly working driver. Having three You need the OF table anyways for module autoload since the I2C core will report a OF module alias. You can only do the I2C fallback trick if your driver can't be build as a module. But even in that case you would be ignoring the vendor. > different names for exactly the same isn't very beautiful IMO. > I agree with you on that. But abusing a table used by another firmware interface isn't beautiful either. So I think the best is to have consistency and always use the same table for the same firmware interface. > I hope you're still not annoyed... > Don't worry for that, it's very hard to get my annoyed :) > Niko > [0]: https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2 Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss wrote: > On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >> >> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss >> wrote: >> > > [snip] > >> But this discussion isn't really related to your patch. I think is >> correct but just said that (b) wasn't a justification to leave the I2C >> table, points (a) and (c) are though. I won't really be convinced that >> the fallback is the correct thing to do or even a good idea. > > > I didn't want to annoy you, I just wanted to understand why you think > fallback is such a bad thing that you call it a bug. And I see, it has its > drawbacks ;-). Anyway, thanks for taking the time to clarify this, > Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say is that I've a strong opinion on this and won't be convinced otherwise :) So for me is a bug because that would mean that either an entry is missing in an OF device table or a DTS has a node with a compatible string without a vendor prefix. > Niko > > [snip] > Best regards, Javier
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss wrote: [snip] >>> >>> Ok, in my opinion it is an elegant way of not bloating the driver when no >>> explicit handling (e.g. reading DT properties) is needed. Just adding an >>> of_device_id doesn't change any driver functionality then but only maps >>> to >>> an already existing i2c_table_id. >>> >> >> I disagree, in the case of OF for example a compatible string is >> composed of both a vendor a device, the complete tuple is what should >> be matched. >> >> But with the fallback, only the device portion would be used so both >> and will match against the i2c device with id >> "bar". It may or may not be correct but the vendor portion is very >> important to disambiguate. > > > Disambiguation via DT implies there is already a name collision in i2c > modalias name space, so adding an of_id would work around this collision for There wouldn't be a name collision between OF modaliases in that case since the vendor would be different (of:N*T*Cfoo,bar and of:N*T*Cbaz,bar). So it wouldn't be a problem for DT-only drivers. > DT enumeration. I2c_board_info driver binding would still be broken. The > right fix would be to remove the name collision. > Yes, for legacy drivers using board files it would be an issue. But that's unrelated to what I'm saying. What I don't think is correct is to ignore the vendor prefix since that's part of the compatible string semantics. In general, I think that there should be consistency between the firmware interface used to register the device, how the module alias uevent is reported to auto-load a driver and how the driver is matched with the device. So drivers supporting DT should have a n OF table (and export it with MODULE_DEVICE_TABLE), drivers supporting ACPI should have a ACPI table and so on. But this discussion isn't really related to your patch. I think is correct but just said that (b) wasn't a justification to leave the I2C table, points (a) and (c) are though. I won't really be convinced that the fallback is the correct thing to do or even a good idea. [snip] >>> >>> It could have been a null pointer and device driver binding (and >>> auto-loading) done just via driver.name. >>> >> >> I'm not sure I understood this comment. > > > What I was trying to say is that if the i2c_device_id table information was > not needed (i.d. only one single id), even the old probe() could be used > without defining an i2c_device_id table, the i2c_device_id* argument to > probe() being a nullptr. > I see, yes that would work too. I didn't authored the .probe_new callback so I don't know if other options were discussed. I also can't remember if the I2C core attempted to probe even without an I2C device ID table, I thought it didn't but I can be wrong. > Niko Best regards, Javier
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
On Wed, Jul 4, 2018 at 1:26 PM, Javier Martinez Canillas wrote: [snip] >> Ok, in my opinion it is an elegant way of not bloating the driver when no >> explicit handling (e.g. reading DT properties) is needed. Just adding an >> of_device_id doesn't change any driver functionality then but only maps to >> an already existing i2c_table_id. >> > > I disagree, in the case of OF for example a compatible string is > composed of both a vendor a device, the complete tuple is what should > be matched. > > But with the fallback, only the device portion would be used so both > and will match against the i2c device with id > "bar". It may or may not be correct but the vendor portion is very > important to disambiguate. > And having a compatible = "bar" is also wrong since the compatible string would lack the vendor prefix. The fact that the I2C (and SPI) core allowed this was what caused the problem in the first place IMO. Best regards, Javier
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
Hi Nikolaus,
On Wed, Jul 4, 2018 at 1:15 PM, Nikolaus Voss
wrote:
> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
[snip]
>> I think Nikolaus is correct, assuming that the driver can be used on
>> legacy
>> platforms that register the I2C devices using board files / platform data.
>> In that case you still need a I2C device ID table for (a) and (c) as he
>> said.
>>
>> I don't buy on (b) though, that's a bug in my opinion. If you register an
>> I2C
>> device via DT then you must have a OF device ID entry that matches the
>> device
>> and the same for ACPI. You can't rely on the I2C device table to do the
>> match.
>
>
> Ok, in my opinion it is an elegant way of not bloating the driver when no
> explicit handling (e.g. reading DT properties) is needed. Just adding an
> of_device_id doesn't change any driver functionality then but only maps to
> an already existing i2c_table_id.
>
I disagree, in the case of OF for example a compatible string is
composed of both a vendor a device, the complete tuple is what should
be matched.
But with the fallback, only the device portion would be used so both
and will match against the i2c device with id
"bar". It may or may not be correct but the vendor portion is very
important to disambiguate.
>>
>> I would also remove the struct i2c_device_id .driver_data fields from the
>> I2C
>> device ID table, since are not used and just makes reading the code
>> confusing
>> (only the struct i2c_device_id .name is used as far as I can see).
>
>
> Valid point, thanks. I will change that.
>
>>
>>> Javier, just a summary of the above. Nikolaus switched one driver to
>>> use ->probe_new() hook and left i2c ID table at the same time.
>>> My understanding that this table is not anymore in use.
>>>
>>> But I have to admit I didn't see entire picture of this. Can you shed a
>>> light?
>>>
>>
>> So to shed some light, in the past even {OF,ACPI}-only drivers needed an
>> I2C ID
>> table because: 1) the .probe callback had a struct i2c_device_id *
>> parameter
>> and 2) the I2C core always reported a modalias of the form i2c: even
>> for
>> devices registered via OF.
>
>
> It could have been a null pointer and device driver binding (and
> auto-loading) done just via driver.name.
>
I'm not sure I understood this comment.
> Niko
>
Best regards,
Javier
Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()
Hi Andy,
On 07/04/2018 12:19 PM, Andy Shevchenko wrote:
> Summon Javier to the discussion.
> For my opinion he is an expert in this topic.
I wouldn't call myself an expert in anything to be honest :)
[snip]
>
>> The table is not used by the driver, but is necessary to
>>
>> a) bind an i2c device declared via i2c_board_info with type field set
>>to one of the names of the i2c_device_id table to this driver
>> b) bind an i2c device declared via DT or ACPI but with no match in of_id/
>>acpi_id table but an i2c_device_id table match to this driver (fallback
>>matching)
>> c) create the right modaliases at compile time for this driver to make
>>module auto-loading work in case of a) and b)
>
I think Nikolaus is correct, assuming that the driver can be used on legacy
platforms that register the I2C devices using board files / platform data.
In that case you still need a I2C device ID table for (a) and (c) as he said.
I don't buy on (b) though, that's a bug in my opinion. If you register an I2C
device via DT then you must have a OF device ID entry that matches the device
and the same for ACPI. You can't rely on the I2C device table to do the match.
I would also remove the struct i2c_device_id .driver_data fields from the I2C
device ID table, since are not used and just makes reading the code confusing
(only the struct i2c_device_id .name is used as far as I can see).
> Javier, just a summary of the above. Nikolaus switched one driver to
> use ->probe_new() hook and left i2c ID table at the same time.
> My understanding that this table is not anymore in use.
>
> But I have to admit I didn't see entire picture of this. Can you shed a light?
>
So to shed some light, in the past even {OF,ACPI}-only drivers needed an I2C ID
table because: 1) the .probe callback had a struct i2c_device_id * parameter
and 2) the I2C core always reported a modalias of the form i2c: even for
devices registered via OF.
The .probe_new callbacks solves (1) and the I2C core now reports of:N*T*Cfoo*
solving (2). So the I2C device table isn't required anymore for {OF,ACPI}-only
drivers, but it's still required for drivers that support legacy board files
that calls i2c_register_board_info() directly. Same for the old .probe callback,
it's needed if struct i2c_device_id .driver_data is used in the probe function.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
[PATCH v2] driver core: add a debugfs entry to show deferred devices
With Device Trees (DT), the dependencies of the devices are defined in the
DT, then the drivers parse that information to lookup the needed resources
that have as dependencies.
Since drivers and devices are registered in a non-deterministic way, it is
possible that a device that is a dependency has not been registered yet by
the time that is looked up.
In this case the driver that requires this dependency cannot probe and has
to defer it. So the driver core adds it to a list of deferred devices that
is iterated again every time that a new driver is probed successfully.
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/devices_deferred
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
This information could be obtained partially by enabling debugging, but it
means that the kernel log has to be parsed and the probe deferral balanced
with the successes. This can be error probe and has to be done in a ad-hoc
manner by everyone who needs to debug these kind of issues.
Since the information is already known by the kernel, just show it to make
it easier to debug.
Signed-off-by: Javier Martinez Canillas
---
Andy, I didn't carry your Reviewed-by tag because it wasn't clear to me
if you had agreed with the patch or not from your last email.
Changes since RFC v1:
- Remove unneeded ret variable from deferred_devs_show()
Changes since RFC v2:
- Use DEFINE_SHOW_ATTRIBUTE() macro.
- Don't propagate debugfs_create_file() error.
- Remove IS_ENABLED(CONFIG_DEBUG_FS) guards.
- Drop RFC prefix.
Changes since v1:
- Better explain in the commit message why this patch is useful.
- Rename deferred_devices entry to devices_deferred.
- Add an exit function and remove the debugfs entry.
drivers/base/dd.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d7281c6..489c484301b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -16,6 +16,7 @@
* Copyright (c) 2007-2009 Novell Inc.
*/
+#include
#include
#include
#include
@@ -53,6 +54,7 @@ static DEFINE_MUTEX(deferred_probe_mutex);
static LIST_HEAD(deferred_probe_pending_list);
static LIST_HEAD(deferred_probe_active_list);
static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static struct dentry *deferred_devices;
static bool initcalls_done;
/*
@@ -224,6 +226,24 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(deferred_devs);
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -233,6 +253,9 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ deferred_devices = debugfs_create_file("devices_deferred", 0444, NULL,
+ NULL, &deferred_devs_fops);
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
@@ -242,6 +265,12 @@ static int deferred_probe_initcall(void)
}
late_initcall(deferred_probe_initcall);
+static void __exit deferred_probe_exit(void)
+{
+ debugfs_remove_recursive(deferred_devices);
+}
+__exitcall(deferred_probe_exit);
+
/**
* device_is_bound() - Check if device is bound to a driver
* @dev: device to check
--
2.17.1
Re: [PATCH] driver core: add a debugfs entry to show deferred devices
[adding Peter Robinson - Fedora IoT Architect to cc list]
On 06/20/2018 10:46 AM, Javier Martinez Canillas wrote:
> On 06/20/2018 12:51 AM, Greg Kroah-Hartman wrote:
>
> [snip]
>
>>> @@ -233,6 +252,9 @@ void device_unblock_probing(void)
>>> */
>>> static int deferred_probe_initcall(void)
>>> {
>>> + debugfs_create_file("deferred_devices", 0444, NULL, NULL,
>>> + &deferred_devs_fops);
>>
>> In the root of debugfs?
>>
>
> I added in the root for lack of a better place. Any suggestion is welcomed.
>
>> Anyway, what about "devices_deferred", to help keep things semi-sane if
>> we have other driver core debugfs entries?
>>
>
> I don't have a strong opinion on the name really, so I'll change it.
>
>> And you don't remove the file ever?
>>
>
> Yeah, I saw that it wasn't removed in other places for debugfs entries
> created by the core since unlike drivers they can't be built as a module
> or re-loaded. But you are right, I'll add an __exitcall to remove there.
>
>> And what is the use of this file? What can you do with this
>> information? Who is going to use it? Don't we have other deferred
>
> This patch is the result of a discussion with Tomeu and Mark (cc'ed) to
> allow https://kernelci.org to test if there was a regression that makes
> drivers to defer their probe.
>
> The problem with the probe deferral mechanism is that you don't have a
> way to distinguish between a valid deferral due a dependency not being
> available yet and a bug (i.e: wrong DTB, config symbol not enabled, etc)
> that prevents the device to eventually being probed.
>
This is not only useful for catching regressions though, Peter also told me
that having this information would save him a lot of time when doing hardware
bringup for ARM devices / IoT platforms.
As mentioned, debugging probe deferral issues caused by drivers not available
or wrong Device Trees is really a PITA. Not all architectures have the luxury
of ACPI / PnP / auto enumerable buses / etc, that hide all this complexity.
So the most information to troubleshoot we have, the better in my opinion.
>> probe debugging somewhere else?
>>
>
> There is some debug yes, but it isn't suitable for the use case I explained.
>
> For start, it only tells you if a given driver for a device was deferred or
> probed correctly while this patch attempts to tell what was left (if any)
> in the queue after the last driver was registered.
>
> Second, is only enabled until late_initcall so it will only print the probe
> deferral for built-in drivers and not for modules. This patch registers the
> debugfs entry after the probe debugging has been disabled.
>
>> thanks,
>>
>> greg k-h
>>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH] driver core: add a debugfs entry to show deferred devices
Hi Andy,
On 06/20/2018 01:43 AM, Andy Shevchenko wrote:
> On Wed, Jun 20, 2018 at 1:51 AM, Greg Kroah-Hartman
> wrote:
>> On Tue, Jun 19, 2018 at 10:59:14PM +0200, Javier Martinez Canillas wrote:
>>> For debugging purposes it may be useful to know what are the devices whose
>>> probe function was deferred. Add a debugfs entry showing that information.
>>>
>>> $ cat /sys/kernel/debug/deferred_devices
>>> 4807.i2c:twl@48:bci
>>> musb-hdrc.0.auto
>>> omapdrm.0
>
>
>> And what is the use of this file? What can you do with this
>> information? Who is going to use it? Don't we have other deferred
>> probe debugging somewhere else?
>
> Indeed.
>
> Javier, have you tried to add 'initcall_debug' to a kernel command
> line followed by 'dyndbg="file drivers/base/dd.c +p"'?
>
I already mentioned this to Greg, but I'll elaborate a little bit. Using these
kernel cmdline options will only tell us when a driver for a device was probed
or deferred but it doesn't tell us what's left in the queue after all drivers
have been registered.
Yes, we could parse the kernel log and do some computation to figure out if a
deferred driver finally got probed, but I don't understand why we can't just
expose the deferred queue if the kernel already has that info and is useful?
But even if we do that, the current debug printouts are only enabled until
late_initcall time. So it won't print deferred probes for drivers registered
by modules:
static void deferred_probe_work_func(struct work_struct *work)
{
...
if (initcall_debug && !initcalls_done)
deferred_probe_debug(dev);
else
bus_probe_device(dev);
...
}
static int deferred_probe_initcall(void)
{
...
initcalls_done = true;
...
}
late_initcall(deferred_probe_initcall);
Again, we could change that but in my opinion we should try to make debug more
easier and this patch is quite trivial. The kernelci folks said that this will
be useful for them and allows to detect regressions on drivers' probe as early
as possible, which I think is very important.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH] driver core: add a debugfs entry to show deferred devices
On 06/20/2018 12:51 AM, Greg Kroah-Hartman wrote:
[snip]
>> @@ -233,6 +252,9 @@ void device_unblock_probing(void)
>> */
>> static int deferred_probe_initcall(void)
>> {
>> +debugfs_create_file("deferred_devices", 0444, NULL, NULL,
>> +&deferred_devs_fops);
>
> In the root of debugfs?
>
I added in the root for lack of a better place. Any suggestion is welcomed.
> Anyway, what about "devices_deferred", to help keep things semi-sane if
> we have other driver core debugfs entries?
>
I don't have a strong opinion on the name really, so I'll change it.
> And you don't remove the file ever?
>
Yeah, I saw that it wasn't removed in other places for debugfs entries
created by the core since unlike drivers they can't be built as a module
or re-loaded. But you are right, I'll add an __exitcall to remove there.
> And what is the use of this file? What can you do with this
> information? Who is going to use it? Don't we have other deferred
This patch is the result of a discussion with Tomeu and Mark (cc'ed) to
allow https://kernelci.org to test if there was a regression that makes
drivers to defer their probe.
The problem with the probe deferral mechanism is that you don't have a
way to distinguish between a valid deferral due a dependency not being
available yet and a bug (i.e: wrong DTB, config symbol not enabled, etc)
that prevents the device to eventually being probed.
> probe debugging somewhere else?
>
There is some debug yes, but it isn't suitable for the use case I explained.
For start, it only tells you if a given driver for a device was deferred or
probed correctly while this patch attempts to tell what was left (if any)
in the queue after the last driver was registered.
Second, is only enabled until late_initcall so it will only print the probe
deferral for built-in drivers and not for modules. This patch registers the
debugfs entry after the probe debugging has been disabled.
> thanks,
>
> greg k-h
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
[PATCH] driver core: add a debugfs entry to show deferred devices
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/deferred_devices
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
Signed-off-by: Javier Martinez Canillas
---
Changes since RFC v1:
- Remove unneeded ret variable from deferred_devs_show()
Changes since RFC v2:
- Use DEFINE_SHOW_ATTRIBUTE() macro.
- Don't propagate debugfs_create_file() error.
- Remove IS_ENABLED(CONFIG_DEBUG_FS) guards.
- Drop RFC prefix.
drivers/base/dd.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d7281c6..8ec9e3cfbe4 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -16,6 +16,7 @@
* Copyright (c) 2007-2009 Novell Inc.
*/
+#include
#include
#include
#include
@@ -224,6 +225,24 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(deferred_devs);
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -233,6 +252,9 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ debugfs_create_file("deferred_devices", 0444, NULL, NULL,
+ &deferred_devs_fops);
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
--
2.17.1
Re: [RFC PATCH v2] driver core: add a debugfs entry to show deferred devices
Hello Andy,
Thanks a lot for your feedback.
On 06/19/2018 09:55 PM, Andy Shevchenko wrote:
> On Tue, Jun 19, 2018 at 10:53 PM, Andy Shevchenko
> wrote:
>> On Tue, Jun 19, 2018 at 9:33 PM, Javier Martinez Canillas
>> wrote:
>>> For debugging purposes it may be useful to know what are the devices whose
>>> probe function was deferred. Add a debugfs entry showing that information.
>
>>> +static int deferred_devs_open(struct inode *inode, struct file *file)
>>> +{
>>> + return single_open(file, deferred_devs_show, inode->i_private);
>>> +}
>>> +
>>> +static const struct file_operations deferred_devs_fops = {
>>> + .owner = THIS_MODULE,
>>> + .open = deferred_devs_open,
>>> + .read = seq_read,
>>> + .llseek = seq_lseek,
>>> + .release = single_release,
>>> +};
>>
>> Isn't this DEFINE_SHOW_ATTRIBUTE() ?
>
Indeed.
> Besides that, you are summoning Greg's dark side :-)
> See below.
>
>>> + if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>>> + deferred_devices = debugfs_create_file("deferred_devices",
>>> + 0444, NULL, NULL,
>>> + &deferred_devs_fops);
>
>>> + if (!deferred_devices)
>>> + return -ENOMEM;
>
> This must not prevent the execution. So, the check introduces actually
> a regression.
>
Fair enough, I'll fix these an re-spin the patch.
>>> + }
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
[RFC PATCH v2] driver core: add a debugfs entry to show deferred devices
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/deferred_devices
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Remove unneeded ret variable from deferred_devs_show()
drivers/base/dd.c | 44
1 file changed, 44 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d7281c6..d95bd4636fc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,6 +224,42 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+#include
+
+static struct dentry *deferred_devices;
+
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return 0;
+}
+
+static int deferred_devs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, deferred_devs_show, inode->i_private);
+}
+
+static const struct file_operations deferred_devs_fops = {
+ .owner = THIS_MODULE,
+ .open = deferred_devs_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -233,6 +269,14 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ if (IS_ENABLED(CONFIG_DEBUG_FS)) {
+ deferred_devices = debugfs_create_file("deferred_devices",
+ 0444, NULL, NULL,
+ &deferred_devs_fops);
+ if (!deferred_devices)
+ return -ENOMEM;
+ }
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
--
2.17.1
[RFC PATCH] driver core: add a debugfs entry to show deferred devices
For debugging purposes it may be useful to know what are the devices whose
probe function was deferred. Add a debugfs entry showing that information.
$ cat /sys/kernel/debug/deferred_devices
4807.i2c:twl@48:bci
musb-hdrc.0.auto
omapdrm.0
Signed-off-by: Javier Martinez Canillas
---
drivers/base/dd.c | 45 +
1 file changed, 45 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d7281c6..98a3ab4a852 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,6 +224,43 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+#include
+
+static struct dentry *deferred_devices;
+
+/*
+ * deferred_devs_show() - Show the devices in the deferred probe pending list.
+ */
+static int deferred_devs_show(struct seq_file *s, void *data)
+{
+ struct device_private *curr;
+ int ret = 0;
+
+ mutex_lock(&deferred_probe_mutex);
+
+ list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
+ seq_printf(s, "%s\n", dev_name(curr->device));
+
+ mutex_unlock(&deferred_probe_mutex);
+
+ return ret;
+}
+
+static int deferred_devs_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, deferred_devs_show, inode->i_private);
+}
+
+static const struct file_operations deferred_devs_fops = {
+ .owner = THIS_MODULE,
+ .open = deferred_devs_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
/**
* deferred_probe_initcall() - Enable probing of deferred devices
*
@@ -233,6 +270,14 @@ void device_unblock_probing(void)
*/
static int deferred_probe_initcall(void)
{
+ if (IS_ENABLED(CONFIG_DEBUG_FS)) {
+ deferred_devices = debugfs_create_file("deferred_devices",
+ 0444, NULL, NULL,
+ &deferred_devs_fops);
+ if (!deferred_devices)
+ return -ENOMEM;
+ }
+
driver_deferred_probe_enable = true;
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
--
2.17.1
Re: [PATCH v2] Documentation: dt-bindings: Explicitly mark Samsung Exynos SoC bindings as unstable
On Mon, May 28, 2018 at 9:13 PM, Krzysztof Kozlowski wrote: > From: Marek Szyprowski > > Samsung Exynos SoCs and boards related bindings evolved since the initial > introduction, but initially the bindings were minimal and a bit incomplete > (they never described all the hardware modules available in the SoCs). > Since then some significant (not fully compatible) changes have been > already committed a few times (like gpio replaced by pinctrl, display ddc, > mfc reserved memory, some core clocks added to various hardware modules, > added more required nodes). > > On the other side there are no boards which have device tree embedded in > the bootloader. Device tree blob is always compiled from the kernel tree > and updated together with the kernel image. > > Thus to avoid further adding a bunch of workarounds for old/missing > bindings, make development of new platforms easier and allow to make > cleanup of the existing code and device tree files, lets mark some > Samsung Exynos SoC platform bindings as unstable. This means that > bindings can may change at any time and users should use the dtb file > compiled from the same kernel source tree as the kernel image. > > Signed-off-by: Marek Szyprowski > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH v2] Input: atmel_mxt_ts - add missing compatible strings to OF device table
Hi Dmitry,
On 05/01/2018 10:57 PM, Dmitry Torokhov wrote:
> From: Javier Martinez Canillas
>
> Commit af503716ac14 ("i2c: core: report OF style module alias for devices
> registered via OF") fixed how the I2C core reports the module alias when
> devices are registered via OF.
>
> But the atmel_mxt_ts driver only has an "atmel,maxtouch" compatible in its
> OF device ID table, so if a Device Tree is using a different one, autoload
> won't be working for the module (the matching works because the I2C device
> ID table is used as a fallback).
>
> So add compatible strings for each of the entries in the I2C device table.
>
> Fixes: af503716ac14 ("i2c: core: report OF style module alias for devices
> registered via OF")
> Reported-by: Enric Balletbo i Serra
> Signed-off-by: Javier Martinez Canillas
> Tested-by: Enric Balletbo i Serra
> [dtor: document which compatibles are deprecated and should not be used]
> Signed-off-by: Dmitry Torokhov
Thanks for taking care of this. Yes, I should had added as deprecated indeed.
Best regards,
Javier
Re: [PATCH v2] regulator: Don't return or expect -errno from of_map_mode()
Hi Doug,
Patch looks good to me, I just have some minor comments.
On Wed, Apr 18, 2018 at 5:31 AM, Douglas Anderson wrote:
> In of_get_regulation_constraints() we were taking the result of
> of_map_mode() (an unsigned int) and assigning it to an int. We were
> then checking whether this value was -EINVAL. Some implementers of
> of_map_mode() were returning -EINVAL (even though the return type of
> their function needed to be unsigned int) because they needed to to
s/to to/to
> signal an error back to of_get_regulation_constraints().
>
> In general in the regulator framework the mode is always referred to
> as an unsigned int. While we could fix this to be a signed int (the
> highest value we store in there right now is 0x8), it's actually
> pretty clean to just define the regulator mode 0x0 (the lack of any
> bits set) as an invalid mode. Let's do that.
>
> Suggested-by: Javier Martinez Canillas
> Fixes: 5e5e3a42c653 ("regulator: of: Add support for parsing initial and
> suspend modes")
> Signed-off-by: Douglas Anderson
> ---
>
> Changes in v2:
> - Use Javier's suggestion of defining 0x0 as invalid
>
> drivers/regulator/cpcap-regulator.c | 2 +-
> drivers/regulator/of_regulator.c| 15 +--
> drivers/regulator/twl-regulator.c | 2 +-
> include/linux/regulator/consumer.h | 1 +
> 4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/cpcap-regulator.c
> b/drivers/regulator/cpcap-regulator.c
> index f541b80f1b54..bd910fe123d9 100644
> --- a/drivers/regulator/cpcap-regulator.c
> +++ b/drivers/regulator/cpcap-regulator.c
> @@ -222,7 +222,7 @@ static unsigned int cpcap_map_mode(unsigned int mode)
> case CPCAP_BIT_AUDIO_LOW_PWR:
> return REGULATOR_MODE_STANDBY;
> default:
> - return -EINVAL;
> + return REGULATOR_MODE_INVALID;
> }
> }
>
> diff --git a/drivers/regulator/of_regulator.c
> b/drivers/regulator/of_regulator.c
> index f47264fa1940..22c02b7a338b 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -124,11 +124,12 @@ static void of_get_regulation_constraints(struct
> device_node *np,
>
> if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
> if (desc && desc->of_map_mode) {
> - ret = desc->of_map_mode(pval);
> - if (ret == -EINVAL)
> + unsigned int mode = desc->of_map_mode(pval);
I think the convention is to always declare local variables at the
start of the function? Although I couldn't find anything in the coding
style document...
> +
> + if (mode == REGULATOR_MODE_INVALID)
> pr_err("%s: invalid mode %u\n", np->name,
> pval);
> else
> - constraints->initial_mode = ret;
> + constraints->initial_mode = mode;
> } else {
> pr_warn("%s: mapping for mode %d not defined\n",
> np->name, pval);
> @@ -163,12 +164,14 @@ static void of_get_regulation_constraints(struct
> device_node *np,
> if (!of_property_read_u32(suspend_np, "regulator-mode",
> &pval)) {
> if (desc && desc->of_map_mode) {
> - ret = desc->of_map_mode(pval);
> - if (ret == -EINVAL)
> + unsigned int mode = desc->of_map_mode(pval);
> +
> + mode = desc->of_map_mode(pval);
You are calling .of_map_mode and assigning the return value twice here.
If you post a new version, feel free to add:
Reviewed-by: Javier Martinez Canillas
Best regards,
Javier
Re: [PATCH] regulator: Fix return type of of_map_mode()
On Tue, Apr 17, 2018 at 7:22 PM, Mark Brown wrote: > On Tue, Apr 17, 2018 at 10:12:04AM -0700, Douglas Anderson wrote: >> In of_get_regulation_constraints() it can clearly be seen that the >> return value of of_map_mode() is assigned to a signed integer. This >> is important because the first thing the regulator core does with this >> value is to compare it to -EINVAL. >> Sorry about that, as Mark mentioned both the regulator device specific and standard modes are a bitmap, and that's why I used an unsigned int as return type. IIRC, at some point in the review someone mentioned error handling and I just blindly added a check for -EINVAL assuming the drivers would return that, but forgot about the return type :( >> Let's fix the return type of all of the current of_map_mode() >> functions. While we're at it, we'll remove one pointless "inline". > > Ah, I see... the thing here is that the mode is always an unsigned int > since it's a bitmask - this goes out beying the use in of_map_mode() and > into all the other APIs. We only actually use 4 bits currently so I > think there's no problem switching to int but it seems we should > probably do that consistently throughout the API so that things don't > get missed later on. Maybe another option could be to add a REGULATOR_MODE_INVALID with value 0x0, and fix the drivers that are returning -EINVAL to return that instead? In of_get_regulation_constraints() we could check for that and propagate -EINVAL. Best regards, Javier
[PATCH] kbuild: rpm-pkg: use kernel-install as a fallback for new-kernel-pkg
The new-kernel-pkg script is only present when grubby is installed, but it may not always be the case. So if the script isn't present, attempt to use the kernel-install script as a fallback instead. Signed-off-by: Javier Martinez Canillas --- scripts/package/mkspec | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/package/mkspec b/scripts/package/mkspec index 61427c6f2209..e05646dc24dc 100755 --- a/scripts/package/mkspec +++ b/scripts/package/mkspec @@ -118,6 +118,8 @@ $S$Mln -sf /usr/src/kernels/$KERNELRELEASE source %preun if [ -x /sbin/new-kernel-pkg ]; then new-kernel-pkg --remove $KERNELRELEASE --rminitrd --initrdfile=/boot/initramfs-$KERNELRELEASE.img + elif [ -x /usr/bin/kernel-install ]; then + kernel-install remove $KERNELRELEASE fi %postun -- 2.14.3
[PATCH] Input: atmel_mxt_ts - add missing compatible strings to OF device table
Commit af503716ac14 ("i2c: core: report OF style module alias for devices
registered via OF") fixed how the I2C core reports the module alias when
devices are registered via OF.
But the atmel_mxt_ts driver only has an "atmel,maxtouch" compatible in its
OF device ID table, so if a Device Tree is using a different one, autoload
won't be working for the module (the matching works because the I2C device
ID table is used as a fallback).
So add compatible strings for each of the entries in the I2C device table.
Fixes: af503716ac14 ("i2c: core: report OF style module alias for devices
registered via OF")
Reported-by: Enric Balletbo i Serra
Signed-off-by: Javier Martinez Canillas
---
Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 6 +-
drivers/input/touchscreen/atmel_mxt_ts.c | 4
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index 23e3abc3fdef..cd43fb8bc2ce 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -1,8 +1,12 @@
Atmel maXTouch touchscreen/touchpad
Required properties:
-- compatible:
+- compatible: Must be one of the following
+atmel,qt602240_ts
+atmel,atmel_mxt_ts
+atmel,atmel_mxt_tp
atmel,maxtouch
+atmel,mXT224
- reg: The I2C address of the device
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7659bc48f1db..e8ef83f168d6 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3238,7 +3238,11 @@ static int __maybe_unused mxt_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
static const struct of_device_id mxt_of_match[] = {
+ { .compatible = "atmel,qt602240_ts", },
+ { .compatible = "atmel,atmel_mxt_ts", },
+ { .compatible = "atmel,atmel_mxt_tp", },
{ .compatible = "atmel,maxtouch", },
+ { .compatible = "atmel,mXT224", },
{},
};
MODULE_DEVICE_TABLE(of, mxt_of_match);
--
2.14.3
Re: [PATCH 1/1] efi/libstub: tpm: zero initialize pointer variables for mixed mode
[adding linux-integrity and tpmdd-devel since this was discussed in these ML too] On 03/13/2018 03:09 PM, Ard Biesheuvel wrote: > As reported by Jeremy, running the new TPM libstub code in mixed mode > (i.e., 64-bit kernel on 32-bit UEFI) results in hangs when invoking > the TCG2 protocol, or when accessing the log_tbl pool allocation. > > The reason turns out to be that in both cases, the 64-bit pointer > variables are not fully initialized by the 32-bit EFI code, and so > we should take care to zero initialize these variables beforehand, > or we'll end up dereferencing bogus pointers. > > Reported-by: Jeremy Cline > Signed-off-by: Ard Biesheuvel > --- > drivers/firmware/efi/libstub/tpm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/tpm.c > b/drivers/firmware/efi/libstub/tpm.c > index da661bf8cb96..13c1edd37e96 100644 > --- a/drivers/firmware/efi/libstub/tpm.c > +++ b/drivers/firmware/efi/libstub/tpm.c > @@ -68,11 +68,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t > *sys_table_arg) > efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; > efi_status_t status; > efi_physical_addr_t log_location, log_last_entry; > - struct linux_efi_tpm_eventlog *log_tbl; > + struct linux_efi_tpm_eventlog *log_tbl = NULL; > unsigned long first_entry_addr, last_entry_addr; > size_t log_size, last_entry_size; > efi_bool_t truncated; > - void *tcg2_protocol; > + void *tcg2_protocol = NULL; > > status = efi_call_early(locate_protocol, &tcg2_guid, NULL, > &tcg2_protocol); > Looks good to me. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: Regression from efi: call get_event_log before ExitBootServices
Hi Hans, On 03/07/2018 12:16 PM, Hans de Goede wrote: > Hi, > > On 07-03-18 09:41, Thiebaud Weksteen wrote: >> Hi, >> >> Thanks for testing and sending this report! This patch relies heavily on >> the functions exposed by the firmware. My first guess would be that some of >> these may not be implemented correctly by the manufacturer. >> >> Could you share more information on this specific device? > > I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel > and I'm not seeing this problem, BIOS settings all default (I loaded > the BIOS defaults to make sure). > >> Do you have any link to the manufacturer website (I found [1] but it is >> based on an ARM CPU)? >> Do you have the option to update your firmware? Is a copy of the firmware >> available from the manufacturer? > > This is a really cheap Windows tablet which was given away for free in > the Netherlands with some home-schooling language courses, or something > similar. > > Both mine and Jeremy tablets come from a website in the Netherlands > where people can buy/sell used goods. > > Most relevant for this discussion I guess is that this device is > based on a Bay Trail Z3735G SoC, on which according to the internets: > https://embedded.communities.intel.com/thread/7868 > > The TPM 2.0 it contains is implemented as part of the TXE firmware. > > Since I cannot reproduce I'm thinking that maybe Jeremy actually has > some log messages in the TPM log, where as mine is empty. Is there a > way to make sure some messages are in there? > The UEFI firmware does some measurements and so does shim. So you should have some event logs. What version of shim are you using? And also would be good to know if it's the same shim version that Jeremy is using. > Regards, > > Hans > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/5 V2] tpm: timeouts revamp
On 03/06/2018 01:17 PM, Jarkko Sakkinen wrote: > On Tue, 2018-03-06 at 11:24 +0200, Tomas Winkler wrote: >> This series cleans up tpm timeouts setting and handling. >> >> First motivation was to fix failures coming from too short timeouts >> for commands that creates keys. >> Key generation may take significant time depending on the underlying >> hardware. Rather than increasing default timeout a new constant is >> added, to not stall too long on regular commands failures. >> >> Second is to define timeouts for new tpm2 commands >> defined in TCG 1.36 spec. > Probably a typo, since the latest TCG spec version is the 1.38 [0]. > Where can we get that specification? I don't have that new > version and couldn't find it from the public internet. > > /Jarkko > [0]: https://trustedcomputinggroup.org/tpm-library-specification/ Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH] i2c: core: report OF style module alias for devices registered via OF
On 03/05/2018 04:30 PM, Wolfram Sang wrote: > >> Is this patch under your radar? Should I keep pushing for it or just give up? > > Applied to for-next, thanks for your patience and your preparational > work needed to get this upstream! > Great, thanks a lot for your help! > We will see in linux-next if there slipped in some new drivers which > need fixing, but we have a full cycle to get them adapted. > Yes, I'll check tomorrow's linux-next to see if I can find these and post patches for them. > And hopefully the benefits will shine through right away. > > Thanks again! > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH] i2c: core: report OF style module alias for devices registered via OF
Hello Wolfram, On Sun, Jan 7, 2018 at 2:17 PM, Javier Martinez Canillas wrote: > > On Sun, Dec 3, 2017 at 10:40 PM, Javier Martinez Canillas > wrote: >> The buses should honor the firmware interface used to register the device, >> but the I2C core reports a MODALIAS of the form i2c: even for I2C >> devices registered via OF. >> >> This means that user-space will never get an OF stype uevent MODALIAS even >> when the drivers modules contain aliases exported from both the I2C and OF >> device ID tables. For example, an Atmel maXTouch Touchscreen registered by >> a DT node with compatible "atmel,maxtouch" has the following module alias: >> >> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >> i2c:maxtouch >> >> So udev won't be able to auto-load a module for an OF-only device driver. >> Many OF-only drivers duplicate the OF device ID table entries in an I2C ID >> table only has a workaround for how the I2C core reports the module alias. >> >> This patch changes the I2C core to report an OF related MODALIAS uevent if >> the device was registered via OF. So for the previous example, after this >> patch, the reported MODALIAS for the Atmel maXTouch will be the following: >> >> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >> of:NtrackpadTCatmel,maxtouch >> >> NOTE: This patch may break out-of-tree drivers that were relying on this >> behavior, and only had an I2C device ID table even when the device >> was registered via OF. There are no remaining drivers in mainline >> that do this, but out-of-tree drivers have to be fixed and define >> a proper OF device ID table to have module auto-loading working. >> >> Signed-off-by: Javier Martinez Canillas >> --- > > Any comments on this patch? > Is this patch under your radar? Should I keep pushing for it or just give up? Best regards, Javier
Re: [PATCH AUTOSEL for 4.9 111/219] ASoc: rt5645: Add OF device ID table
On Mon, Mar 5, 2018 at 11:42 AM, Mark Brown wrote: > On Sat, Mar 03, 2018 at 10:29:00PM +, Sasha Levin wrote: >> From: Javier Martinez Canillas >> >> [ Upstream commit 9ba2da5f5d18daaa365ab5426b05e16f1d114786 ] >> >> The driver doesn't have a struct of_device_id table but supported devices >> are registered via Device Trees. This is working on the assumption that a >> I2C device registered via OF will always match a legacy I2C device ID and >> that the MODALIAS reported will always be of the form i2c:. >> >> But this could change in the future so the correct approach is to have an >> OF device ID table if the devices are registered via OF. > > As the commit message itself says this is not fixing anything, it's > defence against future changes. That's correct. These were just preparatory patches for a change in the I2C subsystem to properly report OF module aliases [0]. So these will only be relevant once [0] is merged, but that patch will never be backported to a stable kernel. [0]: https://patchwork.kernel.org/patch/10089425/ Best regards, Javier
Re: [PATCH v2] tpm: use kmemdup() to copy the event log
On 02/20/2018 12:41 PM, Jarkko Sakkinen wrote: > Replaced kmalloc() + memcpy() in tpm_eventlog_efi.c and > tpm_eventlog_of.c. > > Signed-off-by: Jarkko Sakkinen > --- Looks good to me. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: i2c: core: report OF style module alias for devices registered via OF
Hi Wolfram, On Tue, Jan 16, 2018 at 3:11 PM, Javier Martinez Canillas wrote: > Hello Dmitry, > > On 01/16/2018 02:55 PM, Dmitry Mastykin wrote: >> On Sun, 3 Dec 2017 22:40:50 +0100, Javier Martinez Canillas >> wrote: >> >>> The buses should honor the firmware interface used to register the device, >>> but the I2C core reports a MODALIAS of the form i2c: even for I2C >>> devices registered via OF. >>> >>> This means that user-space will never get an OF stype uevent MODALIAS even >>> when the drivers modules contain aliases exported from both the I2C and OF >>> device ID tables. For example, an Atmel maXTouch Touchscreen registered by >>> a DT node with compatible "atmel,maxtouch" has the following module alias: >>> >>> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >>> i2c:maxtouch >>> >>> So udev won't be able to auto-load a module for an OF-only device driver. >>> Many OF-only drivers duplicate the OF device ID table entries in an I2C ID >>> table only has a workaround for how the I2C core reports the module alias. >>> >>> This patch changes the I2C core to report an OF related MODALIAS uevent if >>> the device was registered via OF. So for the previous example, after this >>> patch, the reported MODALIAS for the Atmel maXTouch will be the following: >>> >>> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >>> of:NtrackpadTCatmel,maxtouch >>> >>> NOTE: This patch may break out-of-tree drivers that were relying on this >>> behavior, and only had an I2C device ID table even when the device >>> was registered via OF. There are no remaining drivers in mainline >>> that do this, but out-of-tree drivers have to be fixed and define >>> a proper OF device ID table to have module auto-loading working. >>> >> >> Hello Javier, >> thank you for your patch! >> > > Thanks a lot for testing! > > Hopefully Wolfram will review/merge this soon, otherwise in the meantime > drivers > are going to add I2C device ID tables just as a workaround or new drivers > won't > include a OF device ID tables since these aren't really required today. > Another gentle ping for this. Best regards, Javier
Re: i2c: core: report OF style module alias for devices registered via OF
Hello Dmitry, On 01/16/2018 02:55 PM, Dmitry Mastykin wrote: > On Sun, 3 Dec 2017 22:40:50 +0100, Javier Martinez Canillas > wrote: > >> The buses should honor the firmware interface used to register the device, >> but the I2C core reports a MODALIAS of the form i2c: even for I2C >> devices registered via OF. >> >> This means that user-space will never get an OF stype uevent MODALIAS even >> when the drivers modules contain aliases exported from both the I2C and OF >> device ID tables. For example, an Atmel maXTouch Touchscreen registered by >> a DT node with compatible "atmel,maxtouch" has the following module alias: >> >> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >> i2c:maxtouch >> >> So udev won't be able to auto-load a module for an OF-only device driver. >> Many OF-only drivers duplicate the OF device ID table entries in an I2C ID >> table only has a workaround for how the I2C core reports the module alias. >> >> This patch changes the I2C core to report an OF related MODALIAS uevent if >> the device was registered via OF. So for the previous example, after this >> patch, the reported MODALIAS for the Atmel maXTouch will be the following: >> >> $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias >> of:NtrackpadTCatmel,maxtouch >> >> NOTE: This patch may break out-of-tree drivers that were relying on this >> behavior, and only had an I2C device ID table even when the device >> was registered via OF. There are no remaining drivers in mainline >> that do this, but out-of-tree drivers have to be fixed and define >> a proper OF device ID table to have module auto-loading working. >> > > Hello Javier, > thank you for your patch! > Thanks a lot for testing! Hopefully Wolfram will review/merge this soon, otherwise in the meantime drivers are going to add I2C device ID tables just as a workaround or new drivers won't include a OF device ID tables since these aren't really required today. I made sure that all the drivers in mainline were fixed before posting this, but haven't checked since then. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v2] iio: adc: max9611: fix module auto-loading
On Sun, Jan 14, 2018 at 11:51 AM, Jonathan Cameron wrote: > On Wed, 10 Jan 2018 12:01:07 +0100 > Javier Martinez Canillas wrote: > >> Hello Jonathan, >> >> On Mon, Jan 1, 2018 at 10:53 AM, Jonathan Cameron wrote: >> >> [snip] >> >> > >> > I may well be missing some subtle detail of course having spent only a few >> > minute looking at this! >> > >> >> Your understanding is correct. This change has nothing to do with >> module autoloading. >> >> The .probe_new callback is only used to avoid requiring an I2C device >> ID table since the old .probe callback has a struct i2c_device_id as a >> parameter and so requires a table even in OF (or ACPI) only drivers. > > OK, then let us revisit this patch once we have an answer on your one > fixing the autoprobing more generally. Until we then we don't know > whether we need to add the i2c table in or not whether this suffices. > That's correct. If the driver is for a device that's OF-only (e.g: will never be used from a non-DT platform) then I don't think we should add a I2C table. Now, if Wolfram nacks my patch then we should add it as a workaround to have auto-loading working. > Jonathan > Best regards, Javier
Re: [PATCH v2] iio: adc: max9611: fix module auto-loading
Hello Dmitry,
On Wed, Jan 10, 2018 at 4:51 AM, Dmitry Mastykin wrote:
> Hello Johnatan,
> May be the issue is the same as Javier Martinez Canillas wrote:
> "What is not correct is to require OF-only drivers to have an I2C
> device ID table just as a workaround to have their modules
> auto-loading working."
> Please see: http://lkml.iu.edu/hypermail/linux/kernel/1712.0/01779.html
> So the problem is not in driver, but in auto-loading procedure.
I've posted [0] a fix for the I2C core some time ago. Could you please
give a try to that and maybe provide your {Tested,Reviewed}-by tag?
[0]: https://patchwork.kernel.org/patch/10089425/
Best regards,
Javier
Re: [PATCH v2] iio: adc: max9611: fix module auto-loading
Hello Jonathan, On Mon, Jan 1, 2018 at 10:53 AM, Jonathan Cameron wrote: [snip] > > I may well be missing some subtle detail of course having spent only a few > minute looking at this! > Your understanding is correct. This change has nothing to do with module autoloading. The .probe_new callback is only used to avoid requiring an I2C device ID table since the old .probe callback has a struct i2c_device_id as a parameter and so requires a table even in OF (or ACPI) only drivers. > Jonathan Best regards, Javier
Re: [PATCH] i2c: core: report OF style module alias for devices registered via OF
Hello Wolfram, On Sun, Dec 3, 2017 at 10:40 PM, Javier Martinez Canillas wrote: > The buses should honor the firmware interface used to register the device, > but the I2C core reports a MODALIAS of the form i2c: even for I2C > devices registered via OF. > > This means that user-space will never get an OF stype uevent MODALIAS even > when the drivers modules contain aliases exported from both the I2C and OF > device ID tables. For example, an Atmel maXTouch Touchscreen registered by > a DT node with compatible "atmel,maxtouch" has the following module alias: > > $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias > i2c:maxtouch > > So udev won't be able to auto-load a module for an OF-only device driver. > Many OF-only drivers duplicate the OF device ID table entries in an I2C ID > table only has a workaround for how the I2C core reports the module alias. > > This patch changes the I2C core to report an OF related MODALIAS uevent if > the device was registered via OF. So for the previous example, after this > patch, the reported MODALIAS for the Atmel maXTouch will be the following: > > $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias > of:NtrackpadTCatmel,maxtouch > > NOTE: This patch may break out-of-tree drivers that were relying on this > behavior, and only had an I2C device ID table even when the device > was registered via OF. There are no remaining drivers in mainline > that do this, but out-of-tree drivers have to be fixed and define > a proper OF device ID table to have module auto-loading working. > > Signed-off-by: Javier Martinez Canillas > --- Any comments on this patch? Best regards, Javier
[PATCH v2 1/3] tpm: delete the TPM_TIS_CLK_ENABLE flag
This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
systems, but the only way this can happen is if the code is not correct.
So it's an unnecessary check that just makes the code harder to read.
Suggested-by: Jarkko Sakkinen
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Jarkko Sakkinen
Tested-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm_tis.c | 15 ---
drivers/char/tpm/tpm_tis_core.c | 2 --
drivers/char/tpm/tpm_tis_core.h | 1 -
3 files changed, 18 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index c847fc69a2fc..4b73e28458e3 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -138,9 +138,6 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data *data,
u32 addr, u16 len,
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
while (len--)
*result++ = ioread8(phy->iobase + addr);
@@ -152,9 +149,6 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data *data,
u32 addr, u16 len,
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
while (len--)
iowrite8(*value++, phy->iobase + addr);
@@ -165,9 +159,6 @@ static int tpm_tcg_read16(struct tpm_tis_data *data, u32
addr, u16 *result)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
*result = ioread16(phy->iobase + addr);
return 0;
@@ -177,9 +168,6 @@ static int tpm_tcg_read32(struct tpm_tis_data *data, u32
addr, u32 *result)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
*result = ioread32(phy->iobase + addr);
return 0;
@@ -189,9 +177,6 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, u32
addr, u32 value)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
iowrite32(value, phy->iobase + addr);
return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index dc3600fc79b7..f2bd99fa8352 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -751,7 +751,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
return;
if (value) {
- data->flags |= TPM_TIS_CLK_ENABLE;
data->clkrun_enabled++;
if (data->clkrun_enabled > 1)
return;
@@ -782,7 +781,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
* sure LPC clock is running before sending any TPM command.
*/
outb(0xCC, 0x80);
- data->flags &= ~TPM_TIS_CLK_ENABLE;
}
}
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index afc50cde1ba6..d5c6a2e952b3 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,7 +86,6 @@ enum tis_defaults {
enum tpm_tis_flags {
TPM_TIS_ITPM_WORKAROUND = BIT(0),
- TPM_TIS_CLK_ENABLE = BIT(1),
};
struct tpm_tis_data {
--
2.14.3
[PATCH v2 0/3] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello,
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
This issue and the propossed solution were discussed in this [2] thread,
and the reporter (Jame Ettle) confirmed that his system works again after
the fix in this series.
The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
Changes since v1:
- Add collected tags
- Drop patch that fixed a bug in the error path since was already fixed.
[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/
[3]: git.infradead.org/users/jjs/linux-tpmdd.git
Best regards,
Javier
Javier Martinez Canillas (3):
tpm: delete the TPM_TIS_CLK_ENABLE flag
tpm: follow coding style for variable declaration in
tpm_tis_core_init()
tpm: only attempt to disable the LPC CLKRUN if is already enabled
drivers/char/tpm/tpm_tis.c | 15 ---
drivers/char/tpm/tpm_tis_core.c | 17 +
drivers/char/tpm/tpm_tis_core.h | 1 -
3 files changed, 13 insertions(+), 20 deletions(-)
--
2.14.3
[PATCH v2 2/3] tpm: follow coding style for variable declaration in tpm_tis_core_init()
The coding style says "use just one data declaration per line (no commas
for multiple data declarations)" so follow this convention.
Suggested-by: Jarkko Sakkinen
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Jarkko Sakkinen
Tested-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm_tis_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index f2bd99fa8352..03daf7017e0f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -803,7 +803,9 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
{
- u32 vendor, intfcaps, intmask;
+ u32 vendor;
+ u32 intfcaps;
+ u32 intmask;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
--
2.14.3
[PATCH v2 3/3] tpm: only attempt to disable the LPC CLKRUN if is already enabled
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
One flaw with the logic is that it assumes that the CLKRUN is always
enabled, and so it unconditionally enables it after a TPM transaction.
But it could be that the CLKRUN# signal was already disabled in the LPC
bus and so after the driver probes, CLKRUN_EN will remain enabled which
may break other devices that are attached to the LPC bus but don't have
support for the CLKRUN protocol.
Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas
Tested-by: James Ettle
Tested-by: Jeffery Miller
Reviewed-by: Jarkko Sakkinen
Tested-by: Jarkko Sakkinen
---
This patch fixes the bug reported for the Fedora kernel [0] and the kernel
bugzilla [1]. The issue and the propossed solution were discussed in this
[2] thread.
[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/
drivers/char/tpm/tpm_tis_core.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 03daf7017e0f..a72a9f03286d 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -747,7 +747,8 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
u32 clkrun_val;
- if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
+ if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
+ !data->ilb_base_addr)
return;
if (value) {
@@ -806,6 +807,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
u32 vendor;
u32 intfcaps;
u32 intmask;
+ u32 clkrun_val;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
@@ -831,6 +833,13 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
ILB_REMAP_SIZE);
if (!priv->ilb_base_addr)
return -ENOMEM;
+
+ clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
+ /* Check if CLKRUN# is already not enabled in the LPC bus */
+ if (!(clkrun_val & LPC_CLKRUN_EN)) {
+ iounmap(priv->ilb_base_addr);
+ priv->ilb_base_addr = NULL;
+ }
}
if (chip->ops->clk_enable != NULL)
--
2.14.3
[PATCH v2] tpm: remove unused data fields from I2C and OF device ID tables
The data field for the entries in the device tables are set but not used.
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Also empty spaces (suggested by Jason Gunthorpe).
drivers/char/tpm/tpm_i2c_infineon.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
b/drivers/char/tpm/tpm_i2c_infineon.c
index 005c38879b2e..c1dd39eaaeeb 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -665,9 +665,9 @@ static int tpm_tis_i2c_init(struct device *dev)
}
static const struct i2c_device_id tpm_tis_i2c_table[] = {
- {"tpm_i2c_infineon", 0},
- {"slb9635tt", 0},
- {"slb9645tt", 1},
+ {"tpm_i2c_infineon"},
+ {"slb9635tt"},
+ {"slb9645tt"},
{},
};
@@ -675,9 +675,9 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
#ifdef CONFIG_OF
static const struct of_device_id tpm_tis_i2c_of_match[] = {
- { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
- { .compatible = "infineon,slb9635tt", .data = (void *)0 },
- { .compatible = "infineon,slb9645tt", .data = (void *)1 },
+ {.compatible = "infineon,tpm_i2c_infineon"},
+ {.compatible = "infineon,slb9635tt"},
+ {.compatible = "infineon,slb9645tt"},
{},
};
MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
--
2.14.3
[PATCH] tpm: remove unused data fields from I2C and OF device ID tables
The data field for the entries in the device tables are set but not used.
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm_i2c_infineon.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
b/drivers/char/tpm/tpm_i2c_infineon.c
index 005c38879b2e..19fbd6a3a8e9 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -665,9 +665,9 @@ static int tpm_tis_i2c_init(struct device *dev)
}
static const struct i2c_device_id tpm_tis_i2c_table[] = {
- {"tpm_i2c_infineon", 0},
- {"slb9635tt", 0},
- {"slb9645tt", 1},
+ {"tpm_i2c_infineon" },
+ {"slb9635tt" },
+ {"slb9645tt" },
{},
};
@@ -675,9 +675,9 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
#ifdef CONFIG_OF
static const struct of_device_id tpm_tis_i2c_of_match[] = {
- { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
- { .compatible = "infineon,slb9635tt", .data = (void *)0 },
- { .compatible = "infineon,slb9645tt", .data = (void *)1 },
+ { .compatible = "infineon,tpm_i2c_infineon" },
+ { .compatible = "infineon,slb9635tt" },
+ { .compatible = "infineon,slb9645tt" },
{},
};
MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
--
2.14.3
Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property
Hello Peter, On Fri, Dec 22, 2017 at 12:07 AM, Peter Rosin wrote: > On 2017-12-21 21:27, Javier Martinez Canillas wrote: >> Hello Peter, >> >> On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin wrote: >>> On 2017-12-21 14:48, Bartosz Golaszewski wrote: >>>> Current description of the compatible property for at24 is quite vague. >>>> >>>> Specify an exact list of accepted compatibles and document the - now >>>> deprecated - strings which were previously used in device tree files. >>> >>> Why is it suddenly deprecated to correctly specify what hardware you >>> have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit. >> >> Sorry but I disagree. >> >> When you specify a compatible string, you are not specifying a >> particular hardware but a device programming model. > > That's not what it says in https://elinux.org/Device_Tree_Usage I think the most up-to-date DT reference is at: https://www.devicetree.org/ > in the "Understanding the compatible Property" section. I quote: > > compatible is a list of strings. The first string in the > list specifies the exact device that the node represents > in the form ",". The following strings > represent other devices that the device is compatible with. > > Pretty clearly talks about devices and not programming models. But > maybe I shouldn't trust that reference? What should I be reading > instead? > For example the latest spec draft (https://github.com/devicetree-org/devicetree-specification/releases/download/v0.2-rc1/devicetree-specification-v0.2-rc1.pdf) says: "The compatible property value consists of one or more strings that define the specific programming model for the device. This list of strings should be used by a client program for device driver selection. The property value consists of a concatenated list of null terminated strings, from most specific to most general. They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices." The recommended format is "manufacturer,model", where manufacturer is a string describing the name of the manufacturer (such as a stock ticker symbol), and model specifies the model number." Example: compatible = "fsl,mpc8641", "ns16550"; In this example, an operating system would first try to locate a device driver that supported fsl,mpc8641. If a driver was not found, it would then try to locate a driver that supported the more general ns16550 device type." >> It's very common to use a compatible string that doesn't match exactly >> the specific hardware used. That's why it's called _compatible_ BTW. > > That's not how I read the above. > That's not how I read it nor my experience with DT, but of course I may be wrong on this. >> For example when a DTS define a UART node with an ns16550 compatible, >> they don't really mean that have a UART IC manufactured by National >> Semiconductor. > > That just tells me that most people are a little bit lazy and ready > to cut a corner or two when they can get away with it. Or that there > is some form of misunderstanding at work... > For example, I usually see that different SoC families from the same vendor use the same compatible string for integrated peripherals just because are the same from a programming model point of view. TI am33xx SoCs use a lot of omap3 compatible strings on their nodes and its similar on Exynos SoCs which are the two ARM SoCs I'm most familiar with. Following your logic that's wrong and instead a new compatible string should be added for the GPIO or pinctrl drivers even when are the same because refer to different devices. >>> Sure, it happens to be compatible with "atmel,24c32", but that is >>> supposed to be written with a fallback as >>> >>> "nxp,24c32", "atmel,24c32" >> >> This isn't a requirement really, systems integrators are free to use >> more than one tuple in the compatible string if >> they want the DTB to be future proof, just in case there's a need for >> a more specific driver or if the programming model happened to not be >> the same at the end. This is usually done for the boards compatible >> string as an example, even when there isn't a struct machine_desc for >> the specific board compatible and only for the SoC family. >> >> So it's OK if you want to define the compatible as "nxp,24c32", >> "atmel,24c32", but that's a general OF concept and not someth
Re: [PATCH v2 1/5] dt-bindings: at24: consistently document the compatible property
Hello Peter, On Thu, Dec 21, 2017 at 5:20 PM, Peter Rosin wrote: > On 2017-12-21 14:48, Bartosz Golaszewski wrote: >> Current description of the compatible property for at24 is quite vague. >> >> Specify an exact list of accepted compatibles and document the - now >> deprecated - strings which were previously used in device tree files. > > Why is it suddenly deprecated to correctly specify what hardware you > have, e.g. "nxp,24c32". In this case the manufacturer is nxp, damnit. Sorry but I disagree. When you specify a compatible string, you are not specifying a particular hardware but a device programming model. It's very common to use a compatible string that doesn't match exactly the specific hardware used. That's why it's called _compatible_ BTW. For example when a DTS define a UART node with an ns16550 compatible, they don't really mean that have a UART IC manufactured by National Semiconductor. > Sure, it happens to be compatible with "atmel,24c32", but that is > supposed to be written with a fallback as > > "nxp,24c32", "atmel,24c32" This isn't a requirement really, systems integrators are free to use more than one tuple in the compatible string if they want the DTB to be future proof, just in case there's a need for a more specific driver or if the programming model happened to not be the same at the end. This is usually done for the boards compatible string as an example, even when there isn't a struct machine_desc for the specific board compatible and only for the SoC family. So it's OK if you want to define the compatible as "nxp,24c32", "atmel,24c32", but that's a general OF concept and not something related to the at24 eeprom driver so I'm not sure if it should be mentioned in the at24 DT binding doc. > if I understand correctly. So, why is that deprecated in this case? > > What if (a few years down the line) it is discovered that some weird > quirk is needed that is only appropriate for nxp chips? > > nxp is of course just an example, pick any manufacturer of eeproms > (supposedly) compatible with the atmel interface. > That's indeed a possibility and the reason why most DT nodes have a more specific before the generic one matched by drivers. I really doubt that in the future it will be found that a more specific compatible string is needed for one manufacturer in this case, specially since the driver didn't even care about the manufacturers until recently. So I think that makes no sense for a driver to support all possible manufacturers as possible compatible strings if all the devices have the same exact programming model. > Cheers, > Peter Best regards, Javier
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello Jeffery, On 12/21/2017 06:25 PM, Jeffery Miller wrote: > On Thu, Dec 21, 2017 at 6:46 AM, Javier Martinez Canillas > wrote: >> >> I meant linux-tpmdd + the 4 patches in this series. > > Javier, > I applied the four patches to the linux-tpmdd master 46dd3b29e328 this > morning and it > fixed the touchpad on my affected machine. > > To be clear, the patchwork patches I applied were: > 10125523 > 10125515 > 10125513 > 10125531 > > I have a slightly different machine (Lenovo Yoga 11e) but found the > same original CLKRUN commit doing a bisect. > Great, thanks for testing! Can I add your Tested-by tag then? Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region
On 12/21/2017 04:39 PM, Shaikh, Azhar wrote: > HI Javier, > > >> -Original Message- >> From: linux-integrity-ow...@vger.kernel.org [mailto:linux-integrity- >> ow...@vger.kernel.org] On Behalf Of Javier Martinez Canillas >> Sent: Thursday, December 21, 2017 4:49 AM >> To: Shaikh, Azhar ; Jason Gunthorpe >> >> Cc: linux-kernel@vger.kernel.org; James Ettle ; Hans de >> Goede ; Arnd Bergmann ; Jarkko >> Sakkinen ; Peter Huewe >> ; Greg Kroah-Hartman >> ; linux-integr...@vger.kernel.org >> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >> memory region >> >> Hello Azhar, >> >> On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: >>> >>> >>>> -Original Message- >>>> From: Jason Gunthorpe [mailto:j...@ziepe.ca] >>>> Sent: Wednesday, December 20, 2017 10:55 AM >>>> To: Javier Martinez Canillas >>>> Cc: linux-kernel@vger.kernel.org; James Ettle ; >>>> Hans de Goede ; Shaikh, Azhar >>>> ; Arnd Bergmann ; Jarkko >>>> Sakkinen ; Peter Huewe >>>> ; Greg Kroah-Hartman >> ; >>>> linux- integr...@vger.kernel.org >>>> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already >>>> unmapped I/O memory region >>>> >>>> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: >>>> >>>>>> The below draft fixes everything except #1. That needs a more >>>>>> thoughtful idea.. >>>>>> >>>>> >>>>> I'll just drop this patch from the series and you can fix all the >>>>> issues in the error / driver removal paths. It's not a dependency >>>>> anyways, I included it just because noticed the issue while reading the >> code. >>>> >>>> Azhar, this means it becomes your problem :) >>>> >>> >>> Sure, I will fix this. >>> Javier you can drop this patch. >>> >> >> On a second though, we should just merge $SUBJECT since is an obvious fix >> for one of the issues. Then you could fix the remaining bugs in error and >> driver removal paths. > > I think you are asking to keep this patch as a part of this series, itself > and not upload separately. Is this correct? Please correct me if I am wrong. > Yes, what I meant is that we should just keep this patch in the series since it fixes one of the resource cleanup bugs. Then you can fix the other issues as a follow-up. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 1/5] dt-bindings: at24: consistently document the compatible property
On Thu, Dec 21, 2017 at 2:28 PM, Bartosz Golaszewski wrote: > 2017-12-21 14:19 GMT+01:00 Javier Martinez Canillas : >> Hello Bartosz, >> >> Nice patch. >> >> On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: >>> Current description of the compatible property for at24 is quite vague. >>> >>> Specify an exact list of accepted compatibles and document the - now >>> deprecated - strings which were previously used in device tree files. >>> >>> Signed-off-by: Bartosz Golaszewski >>> --- >>> Documentation/devicetree/bindings/eeprom/at24.txt | 50 >>> +-- >>> 1 file changed, 28 insertions(+), 22 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt >>> b/Documentation/devicetree/bindings/eeprom/at24.txt >>> index cbc80e194ac6..6ccbd000bfa4 100644 >>> --- a/Documentation/devicetree/bindings/eeprom/at24.txt >>> +++ b/Documentation/devicetree/bindings/eeprom/at24.txt >>> @@ -2,28 +2,34 @@ EEPROMs (I2C) >>> >>> Required properties: >>> >>> - - compatible : should be ",", like these: >>> - >>> - "atmel,24c00", "atmel,24c01", "atmel,24c02", "atmel,24c04", >>> - "atmel,24c08", "atmel,24c16", "atmel,24c32", "atmel,24c64", >>> - "atmel,24c128", "atmel,24c256", "atmel,24c512", "atmel,24c1024" >>> - >>> - "catalyst,24c32" >>> - >>> - "microchip,24c128" >>> - >>> - "ramtron,24c64" >>> - >>> - "renesas,r1ex24002" >>> - >>> - The following manufacturers values have been deprecated: >>> - "at", "at24" >>> - >>> -If there is no specific driver for , a generic >>> -device with and manufacturer "atmel" should be used. >>> -Possible types are: >>> -"24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", >>> "24c64", >>> -"24c128", "24c256", "24c512", "24c1024", "spd" >>> + - compatible: must be one of the following: >>> + >>> +"atmel,24c00", >>> +"atmel,24c01", >>> +"atmel,24c02", >>> +"atmel,24c04", >>> +"atmel,24c08", >>> +"atmel,24c16", >>> +"atmel,24c32", >>> + "atmel,24c64", >>> +"atmel,24c128", >>> +"atmel,24c256", >>> +"atmel,24c512", >>> +"atmel,24c1024" >>> + >>> + NOTE: old compatible strings, such as: >>> + >>> +"catalyst,24c32", >>> +"microchip,24c128", >>> +"ramtron,24c64", >>> +"renesas,r1ex24002", >>> +"at,24c08", >>> +"at24,24c08" >>> + >>> + will still work, but are now deprecated. >>> + >>> + Also: matching by device type alone - while still supported due to >>> + implementation details in I2C core - is deprecated as well. >>> >> >> I don't think that's correct to mention Linux specific implementation >> details in a Device Tree binding. It's supposed to be OS independent >> and in theory the same DT binding could be used in other OS / >> bootloaders. >> >> With that last paragraph removed, feel free to add: >> >> Reviewed-by: Javier Martinez Canillas >> >>>- reg : the I2C address of the EEPROM >>> >> >> Best regards, >> Javier > > How about I just add the no-vendor string examples to the list of > deprecated compatibles above? > yes, that's a good idea. > Best regards, > Bartosz Best regards, Javier
Re: [PATCH 5/5] eeprom: at24: extend the list of chips supported in DT
On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: > Add all supported at24 variants to the of_match table. > > Signed-off-by: Bartosz Golaszewski > --- Reviewed-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH 4/5] dt-bindings: at24: extend the list of supported chips
On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: > Add other variants of at24 EEPROMs we support in the driver to the Again I wouldn't mention the driver here, but instead say that there are variations of the chip that are compatible or something like that. > list of allowed compatible strings. > > Signed-off-by: Bartosz Golaszewski > --- Reviewed-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH 3/5] dt-bindings: at24: fix formatting and style
On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: > Make formatting and style consistent for the entire document. > > This patch doesn't change the content of the binding. > > Signed-off-by: Bartosz Golaszewski > --- Reviewed-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH 2/5] dt-bindings: at24: add a missing compatible
On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: > "atmel,spd" is reported by checkpatch as undocumented in the device > tree bindings. Add it to the list of supported compatible strings. > > Signed-off-by: Bartosz Golaszewski > --- Reviewed-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH 1/5] dt-bindings: at24: consistently document the compatible property
Hello Bartosz, Nice patch. On Thu, Dec 21, 2017 at 2:08 PM, Bartosz Golaszewski wrote: > Current description of the compatible property for at24 is quite vague. > > Specify an exact list of accepted compatibles and document the - now > deprecated - strings which were previously used in device tree files. > > Signed-off-by: Bartosz Golaszewski > --- > Documentation/devicetree/bindings/eeprom/at24.txt | 50 > +-- > 1 file changed, 28 insertions(+), 22 deletions(-) > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt > b/Documentation/devicetree/bindings/eeprom/at24.txt > index cbc80e194ac6..6ccbd000bfa4 100644 > --- a/Documentation/devicetree/bindings/eeprom/at24.txt > +++ b/Documentation/devicetree/bindings/eeprom/at24.txt > @@ -2,28 +2,34 @@ EEPROMs (I2C) > > Required properties: > > - - compatible : should be ",", like these: > - > - "atmel,24c00", "atmel,24c01", "atmel,24c02", "atmel,24c04", > - "atmel,24c08", "atmel,24c16", "atmel,24c32", "atmel,24c64", > - "atmel,24c128", "atmel,24c256", "atmel,24c512", "atmel,24c1024" > - > - "catalyst,24c32" > - > - "microchip,24c128" > - > - "ramtron,24c64" > - > - "renesas,r1ex24002" > - > - The following manufacturers values have been deprecated: > - "at", "at24" > - > -If there is no specific driver for , a generic > -device with and manufacturer "atmel" should be used. > -Possible types are: > -"24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", > "24c64", > -"24c128", "24c256", "24c512", "24c1024", "spd" > + - compatible: must be one of the following: > + > +"atmel,24c00", > +"atmel,24c01", > +"atmel,24c02", > +"atmel,24c04", > +"atmel,24c08", > +"atmel,24c16", > +"atmel,24c32", > +"atmel,24c64", > +"atmel,24c128", > +"atmel,24c256", > +"atmel,24c512", > +"atmel,24c1024" > + > + NOTE: old compatible strings, such as: > + > + "catalyst,24c32", > +"microchip,24c128", > +"ramtron,24c64", > +"renesas,r1ex24002", > +"at,24c08", > +"at24,24c08" > + > + will still work, but are now deprecated. > + > + Also: matching by device type alone - while still supported due to > + implementation details in I2C core - is deprecated as well. > I don't think that's correct to mention Linux specific implementation details in a Device Tree binding. It's supposed to be OS independent and in theory the same DT binding could be used in other OS / bootloaders. With that last paragraph removed, feel free to add: Reviewed-by: Javier Martinez Canillas >- reg : the I2C address of the EEPROM > Best regards, Javier
Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region
Hello Azhar, On 12/20/2017 08:15 PM, Shaikh, Azhar wrote: > > >> -Original Message- >> From: Jason Gunthorpe [mailto:j...@ziepe.ca] >> Sent: Wednesday, December 20, 2017 10:55 AM >> To: Javier Martinez Canillas >> Cc: linux-kernel@vger.kernel.org; James Ettle ; Hans de >> Goede ; Shaikh, Azhar ; >> Arnd Bergmann ; Jarkko Sakkinen >> ; Peter Huewe ; >> Greg Kroah-Hartman ; linux- >> integr...@vger.kernel.org >> Subject: Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O >> memory region >> >> On Wed, Dec 20, 2017 at 07:21:31PM +0100, Javier Martinez Canillas wrote: >> >>>> The below draft fixes everything except #1. That needs a more >>>> thoughtful idea.. >>>> >>> >>> I'll just drop this patch from the series and you can fix all the >>> issues in the error / driver removal paths. It's not a dependency >>> anyways, I included it just because noticed the issue while reading the >>> code. >> >> Azhar, this means it becomes your problem :) >> > > Sure, I will fix this. > Javier you can drop this patch. > On a second though, we should just merge $SUBJECT since is an obvious fix for one of the issues. Then you could fix the remaining bugs in error and driver removal paths. >> Since the patches Jarkko has queued from you introduce oops's you need to >> fix them.. >> >> Jason > > Regards, > Azhar Shaikh > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello James, On 12/21/2017 11:51 AM, James Ettle wrote: > OK, I built a kernel based on: > > - git clone of git.infradead.org/users/jjs/linux-tpmdd.git taken last night > - applying the patch in https://patchwork.kernel.org/patch/10119417/ I meant linux-tpmdd + the 4 patches in this series. > - stock config-4.14.6-300.fc27.x86_64 + make oldconfig > > The tpm modules are loaded and the keyboard is working. > > Note: I won't have access to this machine over the next few weeks. > No worries, there shouldn't be any function changes anyways with the previous patch shared in the other thread and the changes are quite simple so I think it's OK. Again thanks a lot for your help and merry Christmas / happy new year. > Thanks, > James. > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
On 12/20/2017 06:44 PM, Jason Gunthorpe wrote: > On Wed, Dec 20, 2017 at 05:45:16PM +0100, Javier Martinez Canillas wrote: > >> CHP51 says "LPC Clock Control Using the LPC_CLKRUN# May Not Behave As >> Expected" >> and that the implication is that "The SoC may prevent a peripheral device >> from >> successfully requesting the LPC clock". > > Now we are back to the beginning - the LPC_CLKRUN protocol is simply > broken in BSW chipsets, and it has nothing to do with the TPM? > > Intel is trying to work around that broken-ness and still preserve > power management in the case where only the TPM is connected to the > LPC bus.. It is questionable to me if this is even a good idea, or if > Linux is the right place to implement this work around (eg something > in SMM mode may be more appropriate for a chipset bug) > > I think your patch is still the right improvement, if the BIOS turned > the feature off, we should not turn it back on. > Yes, the patch has merits on its own since fixes a flaw in the logic of the original CLKRUN patch. I think we should merge it and then the other issues can be fixed (or rework how the CLKRUN is managed) as follow-ups. > Jason > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag
On 12/20/2017 07:10 PM, Jason Gunthorpe wrote:
> On Wed, Dec 20, 2017 at 03:19:19PM +, Shaikh, Azhar wrote:
>>> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
>>> systems, but the only way this can happen is if the code is not correct.
>>>
>>> So it's an unnecessary check that just makes the code harder to read.
>>
>> This code was implemented as a suggestion from Jason on the previous
>> patches.
>> https://www.spinics.net/lists/linux-integrity/msg00827.html
>
> The concept was to be like ASSERT_RTNL, maybe it just needs a suitably
> named static inline to addrress Javier's readability concerns?
>
I really think is not worth it and pollutes all the tpm_tcg_{read,write}
functions with those is_bsw() and flags checks. Your example is different
since is a core API used by in lot of places in the kernel, but it's not
the case here.
But I don't have a strong opinion either, it was Jarkko who questioned
the value of the flag so I can drop this patch too if you disagree with
the change. I'm mostly interested in PATCH 4/4 that's the actual fix.
> Jason
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region
On 12/20/2017 07:08 PM, Jason Gunthorpe wrote: > On Wed, Dec 20, 2017 at 12:35:35PM +0100, Javier Martinez Canillas wrote: >> The driver maps the I/O memory address to control the LPC bus CLKRUN_EN, >> but on the error path the memory is accessed by the .clk_enable handler >> after this was already unmapped. So only unmap the I/O memory region if >> it will not be used anymore. >> >> Also, the correct thing to do is to cleanup the resources in the inverse >> order that were acquired to prevent issues like these. >> >> Signed-off-by: Javier Martinez Canillas >> >> drivers/char/tpm/tpm_tis_core.c | 7 --- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c >> b/drivers/char/tpm/tpm_tis_core.c >> index c2227983ed88..3455abbb2035 100644 >> +++ b/drivers/char/tpm/tpm_tis_core.c > > Yoiks. This patch is helping but the more I look at this the wronger > everything looks.. > > 1) tpm_chip_unregister makes chip->ops == NULL, so this sequence: > > static int tpm_tis_plat_remove(struct platform_device *pdev) > tpm_chip_unregister(chip); > tpm_tis_remove(chip); > void tpm_tis_remove(struct tpm_chip *chip) > if (chip->ops->clk_enable != NULL) > > Will oops > > 2) tpm_chip_register can also NULL ops in error cases, so this >sequence can oops: > >rc = tpm_chip_register(chip); >if (rc && is_bsw()) >iounmap(priv->ilb_base_addr); > > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, false); > > 3) iounmap should not be split between tpm_tis and tpm_tis_core >Put it at the end of tpm_tis_remove. > > 4) This sequence: > > + return tpm_chip_register(chip); > +out_err: > + tpm_tis_remove(chip); > + return rc; > >Doesn't look right. If tpm_chip_register fails then >tpm_tis_remove will never be called. This was sort of OK when >tpm_tis_remove didn't manage any resources, but now that it does >the above needs fixing too. > Right, I only noticed the issue this patch fixes and (wrongly) assumed the rest was correct. > The below draft fixes everything except #1. That needs a more thoughtful > idea.. > I'll just drop this patch from the series and you can fix all the issues in the error / driver removal paths. It's not a dependency anyways, I included it just because noticed the issue while reading the code. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello Azhar, On 12/20/2017 04:41 PM, Shaikh, Azhar wrote: > > >> -Original Message----- >> From: Javier Martinez Canillas [mailto:javi...@redhat.com] >> Sent: Wednesday, December 20, 2017 7:31 AM >> To: Shaikh, Azhar ; >> alexander.stef...@infineon.com; hdego...@redhat.com; linux- >> ker...@vger.kernel.org >> Cc: ja...@ettle.org.uk; a...@arndb.de; jarkko.sakki...@linux.intel.com; >> peterhu...@gmx.de; j...@ziepe.ca; gre...@linuxfoundation.org; linux- >> integr...@vger.kernel.org >> Subject: Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell >> systems due CLKRUN enabled >> >> Hello Azhar, >> >> On 12/20/2017 04:08 PM, Shaikh, Azhar wrote: >> >> [snip] >> >>>>> >>>>>> It seems that on machines with a PS/2 controller connected to the >>>>>> LPC bus the BIOS is already doing this, so I've a feeling that it >>>>>> not being done on devices with a TPM is a bug in the firmware >>>>> >>>>> Absolutely agree, system integratos should make sure that all the >>>>> devices connected to the LPC either have CLKRUN protocol support and >>>>> is enabled or disable the CLKRUN protocol permanently. >>>> >>>> As far as I understand it, this is exactly the issue here: They know >>>> that there are devices that do not support the CLKRUN protocol (the >>>> TPM in this case), but they still need to enable it to prevent other >>>> issues. So for the TPM to continue to work, CLKRUN needs to be >>>> disabled temporarily while the TPM is active. >>>> >>> >>> Yes that was the reason to have this fix. We needed CLKRUN to be enabled >> for Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN >> (please check this public documentation >> https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15- >> EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 section 2.3 Power >> Management). So as Alexander mentioned CLKRUN is disabled while TPM >> transactions are in progress. >>> >> >> Yes I do understand that. Please read my answer to Alexander's email and >> also my question (and Hans') about keeping the CLKRUN protocol >> permanently disabled. >> > > We had to enable CLKRUN for BSW issues as mentioned here > https://www.intel.com/content/www/us/en/processors/pentium/pentium-celeron-n-series-spec-update.html > on Page 24 CHP 49 and Page 25 CHP 51 > Thanks for the pointer. But it's still not clear to me after reading the mentioned erratas why the CLKRUN protocol must be enabled. CHP49 says "System May Experience Inability to Boot or May Cease Operation" and that it may be related to the LPC circuitry to stop functioning which causes the inability to boot when activity is high for several years. And that the workaround is: "Firmware code changes for LPC and RTC circuitry and mitigations for SD Card circuitry have been identified and may be implemented for this erratum." Is this Firmware code changes to enable the CLKRUN protocol to minimize the LPC bus activity and so prevent the LPC circuitry to stop functioning? CHP51 says "LPC Clock Control Using the LPC_CLKRUN# May Not Behave As Expected" and that the implication is that "The SoC may prevent a peripheral device from successfully requesting the LPC clock". So I would say that CLKRUN protocol should NOT be enabled instead since the CLKRUN# signal is not reliable. Or what am I missing here? Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello Azhar, On 12/20/2017 04:08 PM, Shaikh, Azhar wrote: [snip] >>> >>>> It seems that on machines with a PS/2 controller connected to the >>>> LPC bus the BIOS is already doing this, so I've a feeling that it >>>> not being done on devices with a TPM is a bug in the firmware >>> >>> Absolutely agree, system integratos should make sure that all the >>> devices connected to the LPC either have CLKRUN protocol support and >>> is enabled or disable the CLKRUN protocol permanently. >> >> As far as I understand it, this is exactly the issue here: They know that >> there >> are devices that do not support the CLKRUN protocol (the TPM in this case), >> but they still need to enable it to prevent other issues. So for the TPM to >> continue to work, CLKRUN needs to be disabled temporarily while the TPM is >> active. >> > > Yes that was the reason to have this fix. We needed CLKRUN to be enabled for > Braswell SOC . But the TPM in this case SLB9655 does not support CLKRUN > (please check this public documentation > https://www.infineon.com/dgdl/Infineon-TPM+SLB+9665-DS-v10_15-EN.pdf?fileId=5546d4625185e0e201518b83d9273d87 > section 2.3 Power Management). So as Alexander mentioned CLKRUN is disabled > while TPM transactions are in progress. > Yes I do understand that. Please read my answer to Alexander's email and also my question (and Hans') about keeping the CLKRUN protocol permanently disabled. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello Alexander,
On 12/20/2017 03:07 PM, alexander.stef...@infineon.com wrote:
>> Hi Hans,
>>
>> Thanks a lot for your feedback.
>>
>> On 12/20/2017 12:43 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 20-12-17 12:35, Javier Martinez Canillas wrote:
>>>> Hello,
>>>>
>>>> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell
>> systems")
>>>> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
>>>> signal during TPM transactions.
>>>>
>>>> Unfortunately this breaks other devices that are attached to the LPC bus
>>>> like for example PS/2 mouse and keyboards.
>>>>
>>>> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
>>>> This issue and the propossed solution were discussed in this [2] thread,
>>>> and the reporter (Jame Ettle) confirmed that his system works again after
>>>> the fix in this series.
>>>>
>>>> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
>>>>
>>>> James,
>>>>
>>>> Even when there shouldn't be any functional changes, I included some
>> other
>>>> fixes / cleanups in this series so it would be great if you can test them
>>>> again. I can't because I don't have access to a machine affected by this.
>>>>
>>>> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
>>>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
>>>> [2]: https://patchwork.kernel.org/patch/10119417/
>>>> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>>
>>>> Javier Martinez Canillas (4):
>>>>tpm: fix access attempt to an already unmapped I/O memory region
>>>>tpm: delete the TPM_TIS_CLK_ENABLE flag
>>>>tpm: follow coding style for variable declaration in
>>>> tpm_tis_core_init()
>>>>tpm: only attempt to disable the LPC CLKRUN if is already enabled
>>>>
>>>> drivers/char/tpm/tpm_tis.c | 17 +
>>>> drivers/char/tpm/tpm_tis_core.c | 24 +---
>>>> drivers/char/tpm/tpm_tis_core.h | 1 -
>>>> 3 files changed, 18 insertions(+), 24 deletions(-)
>>>
>>> Note I'm just reading along here, but I'm wondering if both the TPM
>>> and now also some PS/2 controllers need CLK_RUN to be disabled,
>>> why don't we just disable it once permanently and be done with it?
>>>
>>
>> That's the same question I asked to Azhar who authored the patch that
>> added the CLKRUN toggle logic to the driver, but he didn't answer yet:
>>
>> https://www.spinics.net/lists/linux-integrity/msg01107.html
>>
>> My understanding is that by permanently disabling it, then other devices
>> that do support the CLKRUN protocol will continue to work correctly since
>> the CLKRUN# signal is optional and only used if the host LCLK is stopped.
>>
>> The disadvantage though will be that the power management feature of
>> stopping
>> the LPC host LCLK clock when entering into low-power states will not be
>> used.
>>
>>> It seems that on machines with a PS/2 controller connected to
>>> the LPC bus the BIOS is already doing this, so I've a feeling that
>>> it not being done on devices with a TPM is a bug in the firmware
>>
>> Absolutely agree, system integratos should make sure that all the devices
>> connected to the LPC either have CLKRUN protocol support and is enabled
>> or disable the CLKRUN protocol permanently.
>
> As far as I understand it, this is exactly the issue here: They know that
> there are devices that do not support the CLKRUN protocol (the TPM in this
> case), but they still need to enable it to prevent other issues. So for the
> TPM to continue to work, CLKRUN needs to be disabled temporarily while the
> TPM is active.
I do understand that part. What's not clear to me is if those other issues
as you said exist in practice. My understanding from reading the Low Pin
Count Specification is that the CLKRUN# signal is optional and that the
peripherals that support the CLKRUN protocol should still work even when
the CLKRUN_EN is disabled.
With the disadvantage that the system will be less power efficient of
course.
But yes, the reason why I proposed this minimal change as a fix was to
preserve the behavior of the original patch th
Re: [PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hi Hans,
Thanks a lot for your feedback.
On 12/20/2017 12:43 PM, Hans de Goede wrote:
> Hi,
>
> On 20-12-17 12:35, Javier Martinez Canillas wrote:
>> Hello,
>>
>> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
>> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
>> signal during TPM transactions.
>>
>> Unfortunately this breaks other devices that are attached to the LPC bus
>> like for example PS/2 mouse and keyboards.
>>
>> The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
>> This issue and the propossed solution were discussed in this [2] thread,
>> and the reporter (Jame Ettle) confirmed that his system works again after
>> the fix in this series.
>>
>> The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
>>
>> James,
>>
>> Even when there shouldn't be any functional changes, I included some other
>> fixes / cleanups in this series so it would be great if you can test them
>> again. I can't because I don't have access to a machine affected by this.
>>
>> [0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
>> [2]: https://patchwork.kernel.org/patch/10119417/
>> [3]: git.infradead.org/users/jjs/linux-tpmdd.git
>>
>> Best regards,
>> Javier
>>
>>
>> Javier Martinez Canillas (4):
>>tpm: fix access attempt to an already unmapped I/O memory region
>>tpm: delete the TPM_TIS_CLK_ENABLE flag
>>tpm: follow coding style for variable declaration in
>> tpm_tis_core_init()
>>tpm: only attempt to disable the LPC CLKRUN if is already enabled
>>
>> drivers/char/tpm/tpm_tis.c | 17 +
>> drivers/char/tpm/tpm_tis_core.c | 24 +---
>> drivers/char/tpm/tpm_tis_core.h | 1 -
>> 3 files changed, 18 insertions(+), 24 deletions(-)
>
> Note I'm just reading along here, but I'm wondering if both the TPM
> and now also some PS/2 controllers need CLK_RUN to be disabled,
> why don't we just disable it once permanently and be done with it?
>
That's the same question I asked to Azhar who authored the patch that
added the CLKRUN toggle logic to the driver, but he didn't answer yet:
https://www.spinics.net/lists/linux-integrity/msg01107.html
My understanding is that by permanently disabling it, then other devices
that do support the CLKRUN protocol will continue to work correctly since
the CLKRUN# signal is optional and only used if the host LCLK is stopped.
The disadvantage though will be that the power management feature of stopping
the LPC host LCLK clock when entering into low-power states will not be used.
> It seems that on machines with a PS/2 controller connected to
> the LPC bus the BIOS is already doing this, so I've a feeling that
> it not being done on devices with a TPM is a bug in the firmware
Absolutely agree, system integratos should make sure that all the devices
connected to the LPC either have CLKRUN protocol support and is enabled
or disable the CLKRUN protocol permanently.
> there and we should just disable it everywhere (and probably
> find a better place then the TPM driver to do the disabling).
>
Indeed. Touching a global bus configuration in a driver for a single device
isn't safe to say the least. One problem is that we don't have a LPC bus
subsystem so that's why these sort of quirks are done at the driver level.
> Note this is just an observation, I could be completely wrong here,
> but I've a feeling that just disabling CLKRUN all together is the
> right thing to do and that seems like an easier fix to me.
>
I think your observation is correct. The only problem is the power management
feature being disabled as mentioned. Although as you said it seems that most
BIOS do that (as shown by the patch I posted that just avoids toggling the
CLKRUN state if it's already disabled).
> Specifically what worries me is: what if another driver also takes
> the temporarily disable CLK_RUN approach because of similar issues?
> Now we've 2 drivers playing with the CLKRUN state and racing with
> each others.
>
Agreed. You don't even need another driver, if a Braswell system has 2 TPMs
then you have a race condition and one driver could enable the CLKRUN state
while the other driver thinks that's already disabled, and TPM transactions
won't work in that case.
So yeah, it's not a great situation.
> Regards,
>
> Hans
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
[PATCH 4/4] tpm: only attempt to disable the LPC CLKRUN if is already enabled
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
One flaw with the logic is that it assumes that the CLKRUN is always
enabled, and so it unconditionally enables it after a TPM transaction.
But it could be that the CLKRUN# signal was already disabled in the LPC
bus and so after the driver probes, CLKRUN_EN will remain enabled which
may break other devices that are attached to the LPC bus but don't have
support for the CLKRUN protocol.
Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas
Tested-by: James Ettle
---
This patch fixes the bug reported for the Fedora kernel [0] and the kernel
bugzilla [1]. The issue and the propossed solution were discussed in this
[2] thread.
[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_core.c | 13 +++--
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index d0ad9a89b02b..c79193f6d092 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -314,7 +314,7 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
tpm_chip_unregister(chip);
tpm_tis_remove(chip);
- if (is_bsw())
+ if (is_bsw() && priv->ilb_base_addr)
iounmap(priv->ilb_base_addr);
return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 534cd46fdfae..17fb9c600d0e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -742,7 +742,8 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
u32 clkrun_val;
- if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
+ if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
+ !data->ilb_base_addr)
return;
if (value) {
@@ -801,6 +802,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
u32 vendor;
u32 intfcaps;
u32 intmask;
+ u32 clkrun_val;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
@@ -826,6 +828,13 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
ILB_REMAP_SIZE);
if (!priv->ilb_base_addr)
return -ENOMEM;
+
+ clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
+ /* Check if CLKRUN# is already not enabled in the LPC bus */
+ if (!(clkrun_val & LPC_CLKRUN_EN)) {
+ iounmap(priv->ilb_base_addr);
+ priv->ilb_base_addr = NULL;
+ }
}
if (chip->ops->clk_enable != NULL)
@@ -935,7 +944,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, false);
- if (is_bsw())
+ if (is_bsw() && priv->ilb_base_addr)
iounmap(priv->ilb_base_addr);
return rc;
--
2.14.3
[PATCH 1/4] tpm: fix access attempt to an already unmapped I/O memory region
The driver maps the I/O memory address to control the LPC bus CLKRUN_EN, but on the error path the memory is accessed by the .clk_enable handler after this was already unmapped. So only unmap the I/O memory region if it will not be used anymore. Also, the correct thing to do is to cleanup the resources in the inverse order that were acquired to prevent issues like these. Signed-off-by: Javier Martinez Canillas --- drivers/char/tpm/tpm_tis_core.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index c2227983ed88..3455abbb2035 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -923,7 +923,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, rc = tpm_chip_register(chip); if (rc && is_bsw()) - iounmap(priv->ilb_base_addr); + goto out_err; if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false); @@ -931,12 +931,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, return rc; out_err: tpm_tis_remove(chip); - if (is_bsw()) - iounmap(priv->ilb_base_addr); if (chip->ops->clk_enable != NULL) chip->ops->clk_enable(chip, false); + if (is_bsw()) + iounmap(priv->ilb_base_addr); + return rc; } EXPORT_SYMBOL_GPL(tpm_tis_core_init); -- 2.14.3
[PATCH 3/4] tpm: follow coding style for variable declaration in tpm_tis_core_init()
The coding style says "use just one data declaration per line (no commas
for multiple data declarations)" so follow this convention.
Suggested-by: Jarkko Sakkinen
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm_tis_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09da1e1adc40..534cd46fdfae 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -798,7 +798,9 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
{
- u32 vendor, intfcaps, intmask;
+ u32 vendor;
+ u32 intfcaps;
+ u32 intmask;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
--
2.14.3
[PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag
This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell
systems, but the only way this can happen is if the code is not correct.
So it's an unnecessary check that just makes the code harder to read.
Suggested-by: Jarkko Sakkinen
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm_tis.c | 15 ---
drivers/char/tpm/tpm_tis_core.c | 2 --
drivers/char/tpm/tpm_tis_core.h | 1 -
3 files changed, 18 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index d29add49b033..d0ad9a89b02b 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -138,9 +138,6 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data *data,
u32 addr, u16 len,
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
while (len--)
*result++ = ioread8(phy->iobase + addr);
@@ -152,9 +149,6 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data *data,
u32 addr, u16 len,
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
while (len--)
iowrite8(*value++, phy->iobase + addr);
@@ -165,9 +159,6 @@ static int tpm_tcg_read16(struct tpm_tis_data *data, u32
addr, u16 *result)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
*result = ioread16(phy->iobase + addr);
return 0;
@@ -177,9 +168,6 @@ static int tpm_tcg_read32(struct tpm_tis_data *data, u32
addr, u32 *result)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
*result = ioread32(phy->iobase + addr);
return 0;
@@ -189,9 +177,6 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, u32
addr, u32 value)
{
struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data);
- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE))
- WARN(1, "CLKRUN not enabled!\n");
-
iowrite32(value, phy->iobase + addr);
return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 3455abbb2035..09da1e1adc40 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -746,7 +746,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
return;
if (value) {
- data->flags |= TPM_TIS_CLK_ENABLE;
data->clkrun_enabled++;
if (data->clkrun_enabled > 1)
return;
@@ -777,7 +776,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
* sure LPC clock is running before sending any TPM command.
*/
outb(0xCC, 0x80);
- data->flags &= ~TPM_TIS_CLK_ENABLE;
}
}
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index afc50cde1ba6..d5c6a2e952b3 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,7 +86,6 @@ enum tis_defaults {
enum tpm_tis_flags {
TPM_TIS_ITPM_WORKAROUND = BIT(0),
- TPM_TIS_CLK_ENABLE = BIT(1),
};
struct tpm_tis_data {
--
2.14.3
[PATCH 0/4] tpm: fix PS/2 devices not working on Braswell systems due CLKRUN enabled
Hello,
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
The bug was reported to the Fedora kernel [0] and the kernel bugzilla [1].
This issue and the propossed solution were discussed in this [2] thread,
and the reporter (Jame Ettle) confirmed that his system works again after
the fix in this series.
The patches are based on top or Jarkko Sakkinen's linux-tpmdd [3] tree.
James,
Even when there shouldn't be any functional changes, I included some other
fixes / cleanups in this series so it would be great if you can test them
again. I can't because I don't have access to a machine affected by this.
[0]: https://bugzilla.redhat.com/show_bug.cgi?id=1498987
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=197287
[2]: https://patchwork.kernel.org/patch/10119417/
[3]: git.infradead.org/users/jjs/linux-tpmdd.git
Best regards,
Javier
Javier Martinez Canillas (4):
tpm: fix access attempt to an already unmapped I/O memory region
tpm: delete the TPM_TIS_CLK_ENABLE flag
tpm: follow coding style for variable declaration in
tpm_tis_core_init()
tpm: only attempt to disable the LPC CLKRUN if is already enabled
drivers/char/tpm/tpm_tis.c | 17 +
drivers/char/tpm/tpm_tis_core.c | 24 +---
drivers/char/tpm/tpm_tis_core.h | 1 -
3 files changed, 18 insertions(+), 24 deletions(-)
--
2.14.3
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
On 12/19/2017 02:10 PM, Jarkko Sakkinen wrote: > On Tue, Dec 19, 2017 at 12:34:46AM +0100, Javier Martinez Canillas wrote: >> Hello Jason, >> >> On 12/18/2017 09:19 PM, Jason Gunthorpe wrote: >>> On Mon, Dec 18, 2017 at 07:34:29PM +, Shaikh, Azhar wrote: >>> >>>>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the >>>>> LPC bus have to support the CLKRUN protocol. My guess is that on >>>>> some Braswell systems LPC power management is enabled but the TPM >>>>> device doesn't have CLKRUN support. >>>> >>>> I think this is what might be happening here. >>> >>> That makes it a BIOS bug, not a chipset bug, and we shouldn't be >>> trying to fix it like this in Linux. >>> >> >> Indeed, the system integrator should make sure that all peripherals that >> are connected through the LPC bus either support the CLKRUN protocol and >> CLKRUN_EN is enabled or CLKRUN_EN should be disabled. >> >>> Based on the original discussion I always thought this was an Intel >>> chipset bug and applies to all cases. >>> >> >> After thinking about this and with a better understanding of the issue, >> I think we have 2 options (please let me know if I got something wrong): >> >> 1) Leave the code as is and apply the patch I shared with James. In that >>case the CLKRUN protocol will be disabled only during TPM transactions >>and not enabled again after transactions if it wasn't enabled. >> >>This shouldn't affect other peripherals since even if they have CLKRUN >>support, they should work correctly while CLKRUN protocol is disabled. >> >>The disadvantage is that TPM devices that have CLKRUN support (do they >>exist?) will not take the advantage of the power management feature of >>stopping the LPC host LCLK clock during low-power states. > > CLKRUN is enabled after TPM has processed the command so how this could > make power mgmt worse? > I meant if there are Braswell systems with a TPM that _does_ support CLKRUN protocol. Since the current code disables ClKRUN_EN for all Braswell boards. > /Jarkko > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Jarkko,
On 12/19/2017 01:59 PM, Jarkko Sakkinen wrote:
> James, Javier, thank you for sorting this out. I'll just have couple of
> minor comments on the patch.
>
> On Mon, Dec 18, 2017 at 01:22:28PM +0100, Javier Martinez Canillas wrote:
>> +u32 vendor, intfcaps, intmask, clkrun_val;
>
> Could these split into four lines (one declaration per line)?
>
Yes, I didn't like that either but did this way for consistency with the driver.
>> u8 rid;
>> int rc, probe;
>> struct tpm_chip *chip;
>> @@ -772,6 +772,15 @@ int tpm_tis_core_init(struct device *dev, struct
>> tpm_tis_data *priv, int irq,
>> ILB_REMAP_SIZE);
>> if (!priv->ilb_base_addr)
>> return -ENOMEM;
>> +
>> +clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
>> +/* Check if CLKRUN# is already not enabled in the LPC bus */
>
> /*
>
>> +if (!(clkrun_val & LPC_CLKRUN_EN)) {
>> +priv->flags |= TPM_TIS_CLK_ENABLE;
>
> Is the flag added just so surpress those WARN()'s?
>
I believe so. I just included here again for consistency, but I think the flag
and the warns can just go away.
> I've forgot why the WARN()'s even exist assuming that driver is
> functioning correctly.
>
> /Jarkko
>
If you agree with the patch, I can post it as a formal patch on a series that
do these cleanups as preparatory work. I've also noticed a NULL pointer deref
bug in an error path so I'll also fix that too.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Jason, On 12/18/2017 09:19 PM, Jason Gunthorpe wrote: > On Mon, Dec 18, 2017 at 07:34:29PM +, Shaikh, Azhar wrote: > >>> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the >>> LPC bus have to support the CLKRUN protocol. My guess is that on >>> some Braswell systems LPC power management is enabled but the TPM >>> device doesn't have CLKRUN support. >> >> I think this is what might be happening here. > > That makes it a BIOS bug, not a chipset bug, and we shouldn't be > trying to fix it like this in Linux. > Indeed, the system integrator should make sure that all peripherals that are connected through the LPC bus either support the CLKRUN protocol and CLKRUN_EN is enabled or CLKRUN_EN should be disabled. > Based on the original discussion I always thought this was an Intel > chipset bug and applies to all cases. > After thinking about this and with a better understanding of the issue, I think we have 2 options (please let me know if I got something wrong): 1) Leave the code as is and apply the patch I shared with James. In that case the CLKRUN protocol will be disabled only during TPM transactions and not enabled again after transactions if it wasn't enabled. This shouldn't affect other peripherals since even if they have CLKRUN support, they should work correctly while CLKRUN protocol is disabled. The disadvantage is that TPM devices that have CLKRUN support (do they exist?) will not take the advantage of the power management feature of stopping the LPC host LCLK clock during low-power states. 2) Drop the pending CLKRUN patch in linux-tpmdd and revert CLKRUN commit in mainline. And instead of disabling the CLKRUN protocol during the TPM transactions, disable if the CLKRUN_EN is enabled and the system is in a list of systems that have a TPM that doesn't support CLKRUN. This list could be for example a struct dmi_system_id array and match using DMI data on module_init(). The advantage is that TPM devices with CLKRUN protocol support could make use of the CLKRUN power management feature and only systems with a TPM that doesn't support the CLKRUN protocol will disable it. The disadvantage is that the struct dmi_system_id array to match will have to be maintained and every known-to-be-broken system added to it. Thoughts? > Jason > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Azhar, On 12/18/2017 08:34 PM, Shaikh, Azhar wrote: > > >> -Original Message----- >> From: Javier Martinez Canillas [mailto:javi...@redhat.com] >> Sent: Monday, December 18, 2017 11:30 AM >> To: Jason Gunthorpe >> Cc: Jarkko Sakkinen ; James Ettle >> ; linux-integr...@vger.kernel.org; Shaikh, Azhar >> ; linux-kernel@vger.kernel.org; >> james.l.mor...@oracle.com >> Subject: Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on >> Braswell system >> >> Hello Jason, >> >> Thanks a lot for your feedback. >> >> On 12/18/2017 06:55 PM, Jason Gunthorpe wrote: >>> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote: >>>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote: >>>> >>>> [snip] >>>> >>>>> >>>>> James, >>>>> >>>>> Can you please test the following (untested) patch on top of the >>>>> other two mentioned patches to see if it makes a difference for you? >>>>> >>>> >>>> I should had tried to at least compile the patch :) >>> >>> I think this is backwards.. >>> >>> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then >>> TPM shouldn't do anything at all. >>> >>> If CLKRUN_EN is off, then it should try to turn it on/off to save >>> power. >>> >> >> My knowledge of LPC is near to non-existent so I please forgive me if I'm >> wrong, but my understanding is that it's the opposite of what you said. >> >> When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and >> the host LCLK clock may be stopped when entering in some low-power states. >> >> The CLKRUN# signal is then used by peripherals to restart the LCLK clock >> after >> resuming from low-power states to be able to transmit again. >> >> When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus >> and the host LCLK clock is never stopped even when entering in some low- >> power states. >> > > Hi Javier, > > Yes you are correct with the above understanding of the CLKRUN. > Thanks for the confirmation. >> IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC bus >> have to support the CLKRUN protocol. My guess is that on some Braswell >> systems LPC power management is enabled but the TPM device doesn't have >> CLKRUN support. >> > > I think this is what might be happening here. > One question, what happens if the CLKRUN protocol is disabled and never enabled again? My understanding is that the CLKRUN# signal is not required if the host never stops the LCLK clock, and peripherals that do support the CLKRUN protocol should still work if the CLKRUN protocol is disabled and the LCLK isn't stopped. So from what we discussed above, I think it may be correct to completely disable CLKRUN protocol if there's a peripheral that doesn't support it. > Since I do not have an end product to test this on, I managed to get one BSW > Reference Verification Platform(RVP). I flashed the latest Ubuntu (17.10) > which is on 4.13.13 kernel version and does have the patch. > > I was able to use the PS/2 mouse and keyboard immediately after bootup and > also after suspend/resume cycle. The system does have a TPM.(/dev/tpm0 and > /dev/tpmrm0 exist) > It's not clear to me if the RVP system is the same one where you found the issue and lead to the commit that caused the regression. Could you please test what happens if you disable the CLKRUN_EN and never enable it again? Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello James, On 12/18/2017 07:26 PM, James Ettle wrote: > The keyboard and touchpad work OK with the patch quoted below and the earlier > two applied, i.e. the three patches with signatures: > > 667dcc75be864ff4c17cf58891853b7393bba3e2 > db3248e8a036c39141c8f7e9f1cf5c5ae6815f76 > 370d45a34dc8914066a995a3a6d6df1953ea9f60 > > I applied these to a vanilla kernel.org 4.14.7 source using Fedora > 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded. > > Tests: > 1. Keyboard and touchpad work OK on boot - PASS > 2. Still work after suspend/resume - PASS > > Let me know if you want any further tests. > Great, thanks a lot for testing! > Many thanks, > James. > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Jason,
Thanks a lot for your feedback.
On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>>
>>> James,
>>>
>>> Can you please test the following (untested) patch on top of the other two
>>> mentioned patches to see if it makes a difference for you?
>>>
>>
>> I should had tried to at least compile the patch :)
>
> I think this is backwards..
>
> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
> TPM shouldn't do anything at all.
>
> If CLKRUN_EN is off, then it should try to turn it on/off to save
> power.
>
My knowledge of LPC is near to non-existent so I please forgive me if I'm wrong,
but my understanding is that it's the opposite of what you said.
When CLKRUN_EN is SET, power management is ENABLED on the LPC bus and the host
LCLK clock may be stopped when entering in some low-power states.
The CLKRUN# signal is then used by peripherals to restart the LCLK clock after
resuming from low-power states to be able to transmit again.
When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus and the
host LCLK clock is never stopped even when entering in some low-power states.
IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC bus
have to support the CLKRUN protocol. My guess is that on some Braswell systems
LPC power management is enabled but the TPM device doesn't have CLKRUN support.
And that was the motivation of commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol
for Braswell systems") that introduced the regression.
> Perhaps the best work around is to just delete the turning off of
> CLKRUN_EN ? Uses more power but keeps the clock running which should
> keep both TPM and superio happy.
>
But that's exactly the goal of the commit 5e572cab92f0, to disable the CLKRUN
protocol (probably for what I mentioned before). So doing so will cause issues
for those systems again.
What the patch I shared with James does is to avoid disabling the CLKRUN_EN
if this is already disabled. Which should be a no-op anyways but the problem
is that latter it's enabled even when the driver found disabled to start with.
I still don't like the overall solution since it's too error prone. Touching a
global bus configuration in a driver for a single peripheral isn't safe to say
the least.
But regardless of that, the patch I shared has merits on its own since is wrong
to keep the bus configuration in a different state that was before the driver
was probed. And in fact, James reported that it makes his system to work again.
> Jason
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
[snip]
>
> James,
>
> Can you please test the following (untested) patch on top of the other two
> mentioned patches to see if it makes a difference for you?
>
I should had tried to at least compile the patch :)
Updated patch below:
>From 370d45a34dc8914066a995a3a6d6df1953ea9f60 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Mon, 18 Dec 2017 12:56:28 +0100
Subject: [PATCH v2] tpm: only attempt to disable the LPC CLKRUN if is already
enabled
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
One flaw with the logic is that it assumes that the CLKRUN is always
enabled, and so it unconditionally enables it after a TPM transaction.
But it could be that the CLKRUN signal was already disabled in the LPC
bus and so after the driver probes, the signal will remain enabled which
may break other devices transactions since the clocks will be restarted
by the CLKRUN# signal.
Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm_tis_core.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..5f2b1fc2194f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -688,7 +688,8 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
u32 clkrun_val;
- if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
+ if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
+ !data->ilb_base_addr)
return;
if (value) {
@@ -746,7 +747,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
{
- u32 vendor, intfcaps, intmask;
+ u32 vendor, intfcaps, intmask, clkrun_val;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
@@ -772,6 +773,14 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
ILB_REMAP_SIZE);
if (!priv->ilb_base_addr)
return -ENOMEM;
+
+ clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
+ /* Check if CLKRUN# is already not enabled in the LPC bus */
+ if (!(clkrun_val & LPC_CLKRUN_EN)) {
+ priv->flags |= TPM_TIS_CLK_ENABLE;
+ iounmap(priv->ilb_base_addr);
+ priv->ilb_base_addr = NULL;
+ }
}
if (chip->ops->clk_enable != NULL)
@@ -868,7 +877,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
}
rc = tpm_chip_register(chip);
- if (rc && is_bsw())
+ if (rc && is_bsw() && priv->ilb_base_addr)
iounmap(priv->ilb_base_addr);
if (chip->ops->clk_enable != NULL)
--
2.14.3
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>
>>>>> Although probably reverting the offending commits is the right thing to do
>>>>> until a proper solution is proposed.
>>>>
>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>
>>>
>>> I think I should drop the current patch going to 4.16 and revert the old
>>> patch. Do we agree on this?
>>
>> I think the entire is_bsw thing needs to go, clearly manipulating
>> CLKRUN directly was not fully thought out?
>>
>> Hopefully Intel will come up with a fix patch to preserve PS/2
>> functionality and it won't come to a revert..
>
> Agreed.
One flaw I found with the logic is that it assumes that the CLKRUN signal will
always be enabled. And so the driver enables it unconditionally after is probed
(I believe Jason mentioned that before?).
But I wonder if what's causing issues with the PS/2 devices is that the devices
assume the CLKRUN signal is disabled but this changes because of the tpm driver?
James,
Can you please test the following (untested) patch on top of the other two
mentioned patches to see if it makes a difference for you?
>From 72a57eaa425d639d2622339173ff6bf9d9c86ff3 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Mon, 18 Dec 2017 12:56:28 +0100
Subject: [PATCH 1/1] tpm: only attempt to disable the LPC CLKRUN if is already
enabled
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transactions.
Unfortunately this breaks other devices that are attached to the LPC bus
like for example PS/2 mouse and keyboards.
One flaw with the logic is that it assumes that the CLKRUN is always
enabled, and so it unconditionally enables it after a TPM transaction.
But it could be that the CLKRUN signal was already disabled in the LPC
bus and so after the driver probes, the signal will remain enabled which
may break other devices transactions since the clocks will be restarted
by the CLKRUN# signal.
Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/tpm_tis_core.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..42eb2c54494e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -746,7 +746,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
{
- u32 vendor, intfcaps, intmask;
+ u32 vendor, intfcaps, intmask, clkrun_val;
u8 rid;
int rc, probe;
struct tpm_chip *chip;
@@ -772,6 +772,15 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
ILB_REMAP_SIZE);
if (!priv->ilb_base_addr)
return -ENOMEM;
+
+ clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
+ /* Check if CLKRUN# is already not enabled in the LPC bus */
+ if (!(clkrun_val & LPC_CLKRUN_EN)) {
+ priv->flags |= TPM_TIS_CLK_ENABLE;
+ iounmap(priv->ilb_base_addr);
+ priv->ilb_base_addr = NULL;
+ chip->ops->clk_enable = NULL;
+ }
}
if (chip->ops->clk_enable != NULL)
@@ -868,7 +877,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
}
rc = tpm_chip_register(chip);
- if (rc && is_bsw())
+ if (rc && is_bsw() && priv->ilb_base_addr)
iounmap(priv->ilb_base_addr);
if (chip->ops->clk_enable != NULL)
--
2.14.3
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
On 12/16/2017 09:59 PM, Javier Martinez Canillas wrote:
> On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote:
>> On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote:
>>> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote:
>>>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote:
>>>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote:
>>>>>
>>>>>> Although probably reverting the offending commits is the right thing to
>>>>>> do
>>>>>> until a proper solution is proposed.
>>>>>
>>>>> Yes, if a fix is not forthcoming soon Jarkko should revert.
>>>>>
>>>>
>>>> I think I should drop the current patch going to 4.16 and revert the old
>>>> patch. Do we agree on this?
>>>
>>> I think the entire is_bsw thing needs to go, clearly manipulating
>>> CLKRUN directly was not fully thought out?
>>>
>>> Hopefully Intel will come up with a fix patch to preserve PS/2
>>> functionality and it won't come to a revert..
>>
>> Agreed.
>>
>
> Agreed. Although it would be good to have a solution soon, because this has
> been
> broken since v4.13 and v4.15-rc4 is likely to come this weekend. So we may end
> with Braswell system being non-function for 3 kernel releases...
>
> As mentioned I can write a patch to hide this behavior under a Kconfig option
> that's disabled by default (and maybe depend on BROKEN) or a module parameter
> until a proper solution is found.
>
I meant something like the following [0]. That way, it'll be disabled by default
but still users that really need this workaround can do:
$ ./scripts/config --enable EXPERT
$ ./scripts/config --enable CONFIG_TCG_TIS_ENABLE_CLKRUN_QUIRK
What do you think?
[0]:
>From 2ac0d743392ab96d5bf804830e0f0763a253c6e8 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Mon, 18 Dec 2017 11:10:01 +0100
Subject: [PATCH 1/1] tpm: Add Kconfig option to avoid disabling LPC CLKRUN
unconditionally
Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
signal during TPM transations.
Unfortunately this breaks other devices that are attached into the LPC
bus (e.g: PS/2 mouse and keyboards), so the CLKRUN signal shouldn't be
disabled unconditionally.
Until a proper solution is found, add a Kconfig option to explicitly
enable that behavior if needed instead of doing it unconditionally on
all Braswell machines.
Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
Signed-off-by: Javier Martinez Canillas
---
drivers/char/tpm/Kconfig| 10 ++
drivers/char/tpm/tpm_tis_core.c | 2 +-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index a30352202f1f..220bd24e399f 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -43,6 +43,16 @@ config TCG_TIS
within Linux. To compile this driver as a module, choose M here;
the module will be called tpm_tis.
+config TCG_TIS_ENABLE_CLKRUN_QUIRK
+ bool "Enable LPC CLKRUN workaround for Braswell systems TPM" if EXPERT
+ depends on TCG_TIS_CORE && X86
+ default n
+ ---help---
+ On Intel Braswell systems, the Low Pin Count bus CLKRUN signal has to
+ be disabled during TPM devices transactions to operate correctly. This
+ could break other devices attached to the LPC bus (i.e: PS/2 mouse and
+ keyboards) so only enable if the TPM is the only device in the LPC
bus.
+
config TCG_TIS_SPI
tristate "TPM Interface Specification 1.3 Interface / TPM 2.0 FIFO
Interface - (SPI)"
depends on SPI
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e7bd2e750f69..0858c08b3b89 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -688,7 +688,7 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip,
bool value)
struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
u32 clkrun_val;
- if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
+ if (!IS_ENABLED(TCG_TIS_ENABLE_CLKRUN_QUIRK) || !is_bsw())
return;
if (value) {
--
2.14.3
Re: [PATCH] tpm: remove type and name fields from the I2C Infineon OF table entries
Hello Jarkko, On 12/17/2017 05:27 PM, Jarkko Sakkinen wrote: [snip] > > Thank you for the lessons :-) > You are welcome :) > Reviewed-by: Jarkko Sakkinen > Thanks! > /Jarkko > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
On 12/16/2017 06:01 PM, Jarkko Sakkinen wrote: > On Fri, 2017-12-15 at 10:38 -0700, Jason Gunthorpe wrote: >> On Fri, Dec 15, 2017 at 04:56:30PM +0200, Jarkko Sakkinen wrote: >>> On Thu, Dec 14, 2017 at 12:10:52PM -0700, Jason Gunthorpe wrote: >>>> On Thu, Dec 14, 2017 at 08:08:58PM +0100, Javier Martinez Canillas wrote: >>>> >>>>> Although probably reverting the offending commits is the right thing to do >>>>> until a proper solution is proposed. >>>> >>>> Yes, if a fix is not forthcoming soon Jarkko should revert. >>>> >>> >>> I think I should drop the current patch going to 4.16 and revert the old >>> patch. Do we agree on this? >> >> I think the entire is_bsw thing needs to go, clearly manipulating >> CLKRUN directly was not fully thought out? >> >> Hopefully Intel will come up with a fix patch to preserve PS/2 >> functionality and it won't come to a revert.. > > Agreed. > Agreed. Although it would be good to have a solution soon, because this has been broken since v4.13 and v4.15-rc4 is likely to come this weekend. So we may end with Braswell system being non-function for 3 kernel releases... As mentioned I can write a patch to hide this behavior under a Kconfig option that's disabled by default (and maybe depend on BROKEN) or a module parameter until a proper solution is found. > /Jarkko > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH] tpm: remove type and name fields from the I2C Infineon OF table entries
On 12/15/2017 01:03 PM, Jarkko Sakkinen wrote:
> Hi
>
> Some basic questions about DT, which I'm not expert of.
>
> On Mon, Dec 04, 2017 at 06:39:16PM +0100, Javier Martinez Canillas wrote:
>> The commit 21dc02eab989 ("tpm/tpm_i2c_infineon.c: Add OF attributes type
>> and name to the of_device_id table entries") added type and name fields
>> to the OF device ID table entries for the I2C Infineon TPM driver.
>>
>> The only justification for the change in the commit message is that it's
>> probably a good idea to have these fields populated in the OF entries.
>>
>> But I don't think that's true. In fact, I believe that's not correct to
>> add these fields for the following reasons:
>>
>> * The struct of_device_id .type field matches the device_type property
>>in the Device Tree nodes. The ePAPR document says that new use of the
>>property is deprecated and should only be included in cpu and memory
>>nodes for compatibility with the IEEE 1275-derived Device Trees.
>>Also, mainline Device Tree source files don't include this property
>>in the TPM nodes defined.
>>
>> * The struct of_device_id .name field matches the Device Tree node name
>>but the ePAPR document says that the node name should be generic and
>>reflect the function of the device, not it's programming model. So in
>>the case of TPM chips, it should just be "tpm" but the name fields are
>>set in the OF table entries to the actual device model (i.e: slb9645tt).
>
> So why the name field is not changed to "tpm"?
>
We could, but why would do that? Drivers usually don't care about the node name
used by the DTS and most DT bindings are not strict about that. There are some
exceptions (e.g: regulators bindings) but in most cases is up to the developer
to define whatever node name is suitable for that DT node.
In fact, this driver is the first one I have ever seen that sets a .name field.
What most drivers only use to match is the compatible string.
Also, it seems that "tpm" is not the only used node name for TPM device nodes.
At least "tpm_tis" and "vtpm" are used by looking at mainline DTS and bindings
documents. And who knows if an out-of-tree DTB uses "tpm2" for example?
Finally, the other two drivers for I2C TPM devices (atmel and nuvoton) don't
set a .name, so if we change in this driver we probably would need to change
in those too for consistency. And have a proper DT binding doc so users know
that using "tpm" as node name is a requirement.
>> Now, from a practical point of view this means that the OF module aliases
>> for the supported devices include the name and type of the device entries:
>>
>> $ modinfo drivers/char/tpm/tpm_i2c_infineon.ko | grep alias
>> alias: of:Nslb9645ttTtpm*Cinfineon,slb9645ttC*
>> alias: of:Nslb9645ttTtpm*Cinfineon,slb9645tt
>> alias: of:Nslb9635ttTtpm*Cinfineon,slb9635ttC*
>> alias: of:Nslb9635ttTtpm*Cinfineon,slb9635tt
>> alias: of:Ntpm_i2c_infineonTtpm*Cinfineon,tpm_i2c_infineonC*
>> alias: of:Ntpm_i2c_infineonTtpm*Cinfineon,tpm_i2c_infineon
>>
>> But since the device_type isn't set in the tpm DT nodes and the node name
>> is a generic one, the reported MODALIAS when a device is registered via OF
>> won't match the driver's module aliases:
>>
>> $ cat /sys/class/tpm/tpm0/device/modalias
>> of:NtpmTCinfineon,slb9645tt
>>
>> So remove these fields from the OF entries to allow the module aliases to
>> match the MODALIAS reported by the kernel when the device is registered:
>>
>> $ modinfo drivers/char/tpm/tpm_i2c_infineon.ko | grep alias
>> alias: of:N*T*Cinfineon,slb9645ttC*
>> alias: of:N*T*Cinfineon,slb9645tt
>> alias: of:N*T*Cinfineon,slb9635ttC*
>> alias: of:N*T*Cinfineon,slb9635tt
>> alias: of:N*T*Cinfineon,tpm_i2c_infineonC*
>> alias: of:N*T*Cinfineon,tpm_i2c_infineon
>>
>> Signed-off-by: Javier Martinez Canillas
>> ---
>>
>> drivers/char/tpm/tpm_i2c_infineon.c | 21 +++--
>> 1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>> b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 79d6bbb58e39..005c38879b2e 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -675,24 +675,9 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
>>
>> #ifdef CONFIG_OF
>> static const struct of_devi
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello James, On 12/12/2017 10:17 PM, James Ettle wrote: > OK, I built a kernel 4.14.5 from vanilla kernel.org sources using the Fedora > .config (couldn't get the Fedora package to build). > > I see no difference with both [0, 1] patches applied and the tpm modules > loaded -- no keyboard or touchpad. > > [Note: I'm not *consciously* using the TPM for anything, but /dev/tpm0 is > present and have never initialised it. I don't know if anything in Fedora > uses it by default if present.] > On a default Fedora install the TPM isn't used, but the CLKRUN_EN toggling happens for all I/O operations with the TPM and the driver does access the TPM on its probe function. So even if you don't use it, the driver will mess with the CLKRUN signal. Jarkko and Azhar, I wonder what's the solution for this. I understand the quirk is needed for some systems, but on the other hand breaks functionality for other devices. I think that at the very least this should be disabled by default and have a Kconfig option or module parameter to enable if needed. Since as James said, it causes regressions even for people that are not using the TPM device. Although probably reverting the offending commits is the right thing to do until a proper solution is proposed. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [RESEND PATCH] partial revert of "[media] tvp5150: add HW input connectors support"
Hello Mauro,
On 12/14/2017 06:02 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 6 Dec 2017 01:33:05 +0100
> Javier Martinez Canillas escreveu:
>
>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>> added input signals support for the tvp5150, but the approach was found
>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>
>> This left the driver with an undocumented (and wrong) DT parsing logic,
>> so lets get rid of this code as well until the input connectors support
>> is implemented properly.
>>
>> It's a partial revert due other patches added on top of mentioned commit
>> not allowing the commit to be reverted cleanly anymore. But all the code
>> related to the DT parsing logic and input entities creation are removed.
>>
>> Suggested-by: Laurent Pinchart
>> Signed-off-by: Javier Martinez Canillas
>> Acked-by: Laurent Pinchart
>
>>
>> ---
>>
>> This patch was posted about a year ago but was never merged:
>>
>> https://patchwork.kernel.org/patch/9472623/
>
> It was a RFT, on that time.
>
Yes, sorry if it sounded as if I was complaining. I was just mentioning and
part of the patch falling through the cracks is that I also forgot about it.
> I guess I told that before. Maybe not. Anyway, reverting it doesn't seem
> to be the proper fix, as it will break support for existing devices, by
> removing functionality from tvp5150 driver. You should remind that, since
> the code was added, someone could be already using it, as all it is
I'm not sure about this. What I'm removing is basically dead code (unless
someone is using an undocumented Device Tree binding), since the DT binding
got already removed by commit 31e717dba1e1 ("[media] Revert "[media] tvp5150:
document input connectors DT bindings").
> needed is to have some dtb. Also, it gets rid of a lot of good work for
> no good reason. Reinserting them later while preserving the code
> copyrights could be painful.
>
I would normally agree with you, although I think that in this particular case
is better to just revert this (unused) code for the reasons I mentioned above.
But don't really have a strong opinion on this, so I'm OK with either approach.
> IMHO, the best here is to move ahead, agreeing with a DT structure
> that represents the connectors and then change the driver to
> implement it, if needed.
>
There was some agreement on the DT binding, it's just that I never found time
to implement the logic in the driver. Let's see if I can get some during the
winter holidays and finally fix this.
> Thanks,
> Mauro
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Jarkko,
On 12/14/2017 12:21 PM, Jarkko Sakkinen wrote:
> On Mon, Dec 11, 2017 at 07:37:29PM +, James Ettle wrote:
>> Hello,
>>
>> [First: Apologies if cross-posting from Kernel.org BZ is bad form; my
>> distro BZ advised I post this to your mailing list as well.]
>>
>> Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU
>> breaks PS/2 keyboard and mouse. They just don't respond until after a
>> suspend/resume cycle, and after that they later stop after a while.
>>
>> I have confirmed this by blacklisting tpm modules. I noticed this
>> first with kernel 4.13, and have bisected it down to:
>
> In my GIT tree there is now:
>
> commit db3248e8a036c39141c8f7e9f1cf5c5ae6815f76 (HEAD -> next, origin/next,
> origin/master, master)
> Author: Azhar Shaikh
> Date: Wed Dec 6 17:38:10 2017 -0800
>
> tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
>
> Commit 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
> systems") disabled CLKRUN protocol during TPM transactions and re-enabled
> once the transaction is completed. But there were still some corner cases
> observed where, reading of TPM header failed for savestate command
> while going to suspend, which resulted in suspend failure.
> To fix this issue keep the CLKRUN protocol disabled for the entire
> duration of a single TPM command and not disabling and re-enabling
> again for every TPM transaction. For the other TPM accesses outside
> TPM command flow, add a higher level of disabling and re-enabling
> the CLKRUN protocol, instead of doing for every TPM transaction.
>
> Fixes: 5e572cab92f0bb5 ("tpm: Enable CLKRUN protocol for Braswell
> systems")
>
> Signed-off-by: Azhar Shaikh
> Reviewed-by: Jarkko Sakkinen
> Tested-by: Jarkko Sakkinen
> Signed-off-by: Jarkko Sakkinen
>
> Can you try this?
>
I already suggested the same [0], but James said that made no difference [1].
> /Jarkko
>
[0]: https://lkml.org/lkml/2017/12/12/268
[1]: https://lkml.org/lkml/2017/12/12/1127
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello Jason, On 12/12/2017 07:57 PM, Jason Gunthorpe wrote: > On Tue, Dec 12, 2017 at 12:38:00PM +0100, Javier Martinez Canillas wrote: > >> I'm not familiar with LPC so please let me know if my assumptions are wrong, >> but I find suspicious that a driver for a single device attached to the bus >> can control the CLKRUN# signal which AFAIU may be needed for other devices. > > My understanding was that the fix was to force CLKRUN on before > starting a LPC transaction for TPM. > Yes, I got that part from the commit message and the thread when was posted. > I guess the issues is blindly assuming that CLKRUN started out as off. > It needs to remember the old setting and put it back, and hope against > all hope there isn't another thread mucking with it. > This is the part that worries me, and it seems that assumption was wrong due the reported issues with other PS/2 devices attached to the LPC bus. > Guessing that on a system with a LPC connected superio controller for > PS/2 that CLKRUN will be always high??? > I'm not really familiar with the LPC bus trasmision protocol to comment on this. But if different devices rely on the CLKRUN state, then as you said it needs to remember the old setting and also some synchronization mechanism should be used to serialize access to it and prevent a race? > Jason > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system
Hello James, On 12/11/2017 08:37 PM, James Ettle wrote: > Hello, > > [First: Apologies if cross-posting from Kernel.org BZ is bad form; my distro > BZ advised I post this to your mailing list as well.] > > Situation: enabling TPM on a Clevo W510LU with an Intel N3160 CPU breaks PS/2 > keyboard and mouse. They just don't respond until after a suspend/resume > cycle, and after that they later stop after a while. > > I have confirmed this by blacklisting tpm modules. I noticed this first with > kernel 4.13, and have bisected it down to: > > 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad is the first bad commit > commit 5e572cab92f0bb56ca1e6e5ee4d807663a7ccbad > Author: Azhar Shaikh > Date: Sun Jun 18 19:17:59 2017 -0700 > > tpm: Enable CLKRUN protocol for Braswell systems > > To overcome a hardware limitation on Intel Braswell systems, > disable CLKRUN protocol during TPM transactions and re-enable > once the transaction is completed. > > Signed-off-by: Azhar Shaikh > Reviewed-by: Jarkko Sakkinen > Tested-by: Jarkko Sakkinen > Signed-off-by: Jarkko Sakkinen > Signed-off-by: James Morris > > :04 04 5437c91886cb62c497255f2c60dbedd7268ab50d > 1863a1738ded35a817aad52f9f2b451bd43623d7 Mdrivers > > Currently in Kernel.org bugzilla 197287. > > Please let me know if you need any further info. > I don't have access to a Braswell machine to reproduce this. I tried to do it on different Intel systems by modifying is_bsw() to always return true, but that didn't work either. They work correctly even when CLKRUN_EN is toggled. I'm not familiar with LPC so please let me know if my assumptions are wrong, but I find suspicious that a driver for a single device attached to the bus can control the CLKRUN# signal which AFAIU may be needed for other devices. So that would explain why the mentioned commit causes issues for PS/2 mouse and keyboards, since these are attached to the LPC bus and may rely on the CLKRUN# signal for proper operation. I see that the following [0,1] patches for the tpm_tis driver landed a few days ago, they change how the CLKRUN protocol is enabled/disabled. Instead of doing it per each TPM transaction, it does it once for the duration of a TPM command. Not sure if that will make things better or worse for you, but it would be good to try in case it makes a difference. [0]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/667dcc75be864ff4c17cf58891853b7393bba3e2 [1]: http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/db3248e8a036c39141c8f7e9f1cf5c5ae6815f76 > Many thanks, > James. > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v2] ARM: dts: exynos: Enable Mixer node for Exynos5800 Peach Pi machine
[adding Inki to cc list]
Hello Guillaume,
On 12/12/2017 11:43 AM, Guillaume Tucker wrote:
> On 12/12/17 10:17, Marek Szyprowski wrote:
>> Hi Krzysztof,
>>
>> On 2017-12-12 11:09, Krzysztof Kozlowski wrote:
>>> On Tue, Dec 12, 2017 at 10:55 AM, Krzysztof Kozlowski
>>> wrote:
>>>> On Tue, Dec 12, 2017 at 8:42 AM, Javier Martinez Canillas
>>>> wrote:
>>>>> Commit 1cb686c08d12 ("ARM: dts: exynos: Add status property to Exynos 542x
>>>>> Mixer nodes") disabled the Mixer node by default in the DTSI and enabled
>>>>> for each Exynos 542x DTS. But unfortunately it missed to enable it for the
>>>>> Exynos5800 Peach Pi machine, since the 5800 is also an 542x SoC variant.
>>>>>
>>>>> Fixes: 1cb686c08d12 ("ARM: dts: exynos: Add status property to Exynos
>>>>> 542x Mixer nodes")
>>>>> Signed-off-by: Javier Martinez Canillas
>>>>> Acked-by: Marek Szyprowski
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Remove RFT tag.
>>>> Thanks guys! However I still would like to see a tested-by for this on
>>>> Peach Pi (AFAIU, Marek's only acked the code/solution).
>>> On the other hand I could just apply it for my for-next branch and
>>> we'll see if it fixes kernel-ci boot tests... Not a nice way of
>>> testing but apparently no one has Peach Pi.
>>
>> Frankly, I don't expect that this will solve the boot hang issue on PeachPi.
>> However it should at least hide the unbalanced regulator issue.
>
> We have a peach-pi in our LAVA lab so I've tested it and
> actually, it does fix the hang on v4.15-rc3:
>
>https://lava.collabora.co.uk/scheduler/job/1019877
>https://lava.collabora.co.uk/scheduler/job/1019878
>
> I ran it twice and it booted both times. I also ran the same
> boot tests with the same kernel but the dtb from v4.15-rc3
> without the fix to double check and these failed:
>
>https://lava.collabora.co.uk/scheduler/job/1019879
>https://lava.collabora.co.uk/scheduler/job/1019880
>
>
> Tested-by: Guillaume Tucker
>
>
> Thanks for the fix!
>
Thanks for testing!
Now, I think the exynos-drm driver should cope with an incorrect DTB and
don't crash the machine, the driver should only fail to probe and lead
to a working machine with no display.
But as mentioned, that's a different issue and orthogonal to the DTS fix.
> Guillaume
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
