Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
On Thu, Apr 01, 2021 at 09:22:02PM +0300, Andy Shevchenko wrote: > On Thu, Apr 01, 2021 at 09:06:17PM +0300, Mika Westerberg wrote: > > On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote: > > > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote: > > > > Am Mon, 8 Mar 2021 14:20:16 +0200 > > > > schrieb Andy Shevchenko : > > > > > > ... > > > > > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR > > > > > + * @pdev:PCI device to get a PCI bus to communicate with > > > > > + * @devfn: PCI slot and function to communicate with > > > > > + * @mem: memory resource to be filled in > > > > > > > > Do we really need that many arguments to it? > > > > > > > > Before i had, in a platform driver that never had its own pci_dev or bus > > > > > > > > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0)); > > > > if (res-start == 0) > > > > return -ENODEV; > > > > > > > > So helper only asked for the devfn, returned base and no dedicated > > > > error code. > > > > > > > > With this i need > > > > > > > > struct pci_bus *bus = pci_find_bus(0, 0); > > > > struct pci_dev *pci_dev = bus->self; > > > > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0); > > > > > > What confuses me is the use for SPI NOR controller on Broxton. And I think > > > we actually can indeed hide all this under the hood by exposing P2SB to > > > the OS. > > > > > > Mika, what do you think? > > > > Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD) > > driver to that? > > Not MFD, SPI NOR (if I understood correctly the code in MFD driver for SPI NOR > in regards to P2SB case). I mean a new MFD driver that binds to the P2SB and that one then exposes the stuff needed by the SPI-NOR driver.
Re: [PATCH v1 3/7] PCI: New Primary to Sideband (P2SB) bridge support library
On Thu, Apr 01, 2021 at 06:43:11PM +0300, Andy Shevchenko wrote: > On Sat, Mar 13, 2021 at 10:45:57AM +0100, Henning Schild wrote: > > Am Mon, 8 Mar 2021 14:20:16 +0200 > > schrieb Andy Shevchenko : > > ... > > > > + * pci_p2sb_bar - Get Primary to Sideband bridge (P2SB) device BAR > > > + * @pdev:PCI device to get a PCI bus to communicate with > > > + * @devfn: PCI slot and function to communicate with > > > + * @mem: memory resource to be filled in > > > > Do we really need that many arguments to it? > > > > Before i had, in a platform driver that never had its own pci_dev or bus > > > > res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0)); > > if (res-start == 0) > > return -ENODEV; > > > > So helper only asked for the devfn, returned base and no dedicated > > error code. > > > > With this i need > > > > struct pci_bus *bus = pci_find_bus(0, 0); > > struct pci_dev *pci_dev = bus->self; > > unsigned int magic_i_do_not_want = PCI_DEVFN(13, 0); > > What confuses me is the use for SPI NOR controller on Broxton. And I think > we actually can indeed hide all this under the hood by exposing P2SB to the > OS. > > Mika, what do you think? Not sure I follow. Do you mean we force unhide P2SB and then bind (MFD) driver to that?
Re: [PATCH] resource: Prevent irqresource_disabled() from erasing flags
Hi,
On Tue, Mar 30, 2021 at 05:09:42PM +0200, Rafael J. Wysocki wrote:
> On 3/29/2021 9:52 PM, Angela Czubak wrote:
> > Do not overwrite flags as it leads to erasing triggering and polarity
> > information which might be useful in case of hard-coded interrupts.
> > This way the information can be read later on even though mapping to
> > APIC domain failed.
> >
> > Signed-off-by: Angela Czubak
> > ---
> > Some Chromebooks use hard-coded interrupts in their ACPI tables.
> > This is an excerpt as dumped on Relm:
> >
> > ...
> > Name (_HID, "ELAN0001") // _HID: Hardware ID
> > Name (_DDN, "Elan Touchscreen ") // _DDN: DOS Device Name
> > Name (_UID, 0x05) // _UID: Unique ID
> > Name (ISTP, Zero)
> > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource
> > Settings
> > {
> > Name (BUF0, ResourceTemplate ()
> > {
> > I2cSerialBusV2 (0x0010, ControllerInitiated,
> > 0x00061A80,
> > AddressingMode7Bit, "\\_SB.I2C1",
> > 0x00, ResourceConsumer, , Exclusive,
> > )
> > Interrupt (ResourceConsumer, Edge, ActiveLow,
> > Exclusive, ,, )
> > {
> > 0x00B8,
> > }
> > })
> > Return (BUF0) /* \_SB_.I2C1.ETSA._CRS.BUF0 */
> > }
> > ...
> >
> > This interrupt is hard-coded to 0xB8 = 184 which is too high to be mapped
> > to IO-APIC, so no triggering information is propagated as
> > acpi_register_gsi()
> > fails and irqresource_disabled() is issued, which leads to erasing
> > triggering
> > and polarity information.
> > If that function added its flags instead of overwriting them the correct IRQ
> > type would be set even for the hard-coded interrupts, which allows device
> > driver
> > to retrieve it.
> > Please, let me know if this kind of modification is acceptable.
>
> From the quick look it should not be problematic, but it needs to be checked
> more carefully.
>
> Mika, what do you think?
I think it makes sense. We still set IORESOURCE_DISABLED unconditionally
so this should not cause issues. In theory at least :)
> > include/linux/ioport.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index 55de385c839cf..647744d8514e0 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -331,7 +331,7 @@ static inline void irqresource_disabled(struct resource
> > *res, u32 irq)
> > {
> > res->start = irq;
> > res->end = irq;
> > - res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
> > + res->flags |= IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
> > }
> > extern struct address_space *iomem_get_mapping(void);
>
Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
On Mon, Mar 29, 2021 at 09:07:18AM +0300, Dan Carpenter wrote:
> After the device_register() succeeds, then the correct way to clean up
> is to call device_unregister(). The unregister calls both device_del()
> and device_put(). Since this code was only device_del() it results in
> a memory leak.
>
> Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
> Signed-off-by: Dan Carpenter
> ---
> This is from a new static checker warning. Not tested. With new
> warnings it's also possible that I have misunderstood something
> fundamental so review carefully etc.
>
> drivers/thunderbolt/retimer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Both patches applied to thunderbolt.git/fixes. Thanks Dan!
Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
On Mon, Mar 29, 2021 at 11:54:05AM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 29, 2021 at 05:43:23PM +0300, Mika Westerberg wrote:
>
> > The nvm is a separate (physical Linux) device that gets added under this
> > one. It cannot be added before AFAICT.
>
> Hum, yes, but then it is odd that a parent is holding sysfs attributes
> that refer to a child.
Well the child (NVMem) comes from completely different subsystem that
does not have a concept of "authentication" or anythin similar. This is
what we add on top. We actually exposer two NVMem devices under each
retimer: one that is the current active one, and then the one that is
used to write the new firmware image.
> > The code you refer actually looks like this:
> >
> > static ssize_t nvm_authenticate_store(struct device *dev,
> > struct device_attribute *attr, const char *buf, size_t count)
> > {
> > ...
> > if (!mutex_trylock(>tb->lock)) {
> > ret = restart_syscall();
> > goto exit_rpm;
> > }
>
> Is that lock held during tb_retimer_nvm_add() I looked for a bit and
> didn't find something. So someplace more than 4 call site above
> mandatory locking is being held?
Yes it is. It is called from tb_scan_port() where that lock is held.
> static void tb_retimer_remove(struct tb_retimer *rt)
> {
> dev_info(>dev, "retimer disconnected\n");
> tb_nvm_free(rt->nvm);
> device_unregister(>dev);
> }
>
> Here too?
Yes.
> And this is why it is all trylock because it deadlocks with unregister
> otherwise?
I tried to explain it in 09f11b6c99fe ("thunderbolt: Take domain lock in
switch sysfs attribute callbacks"), except that at that time we did not
have retimers exposed but the same applies here too.
Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
Hi,
On Mon, Mar 29, 2021 at 10:02:20AM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 29, 2021 at 09:07:18AM +0300, Dan Carpenter wrote:
> > After the device_register() succeeds, then the correct way to clean up
> > is to call device_unregister(). The unregister calls both device_del()
> > and device_put(). Since this code was only device_del() it results in
> > a memory leak.
> >
> > Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
> > Signed-off-by: Dan Carpenter
> > ---
> > This is from a new static checker warning. Not tested. With new
> > warnings it's also possible that I have misunderstood something
> > fundamental so review carefully etc.
>
> It looks OK to me
I agree too.
> Reviewed-by: Jason Gunthorpe
Thanks for the review!
> This also highlights the code has an ordering issue too, it calls
> device_register() then goes to do tb_retimer_nvm_add() however
> device_register() makes sysfs attributes visible before the rt->nvm is
> initialized and this:
>
> static ssize_t nvm_authenticate_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> if (!rt->nvm) {
>
> Isn't strong enough to close the potential racing. The nvm should be
> setup before device_register and all the above tests in the sysfs
> deleted so we can rely on the CPU barriers built into
> device_register() for correctness.
>
> [which is a general tip, be very suspicious if device_register() is
> being error unwound]
The nvm is a separate (physical Linux) device that gets added under this
one. It cannot be added before AFAICT.
The code you refer actually looks like this:
static ssize_t nvm_authenticate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
...
if (!mutex_trylock(>tb->lock)) {
ret = restart_syscall();
goto exit_rpm;
}
if (!rt->nvm) {
ret = -EAGAIN;
goto exit_unlock;
}
Idea here is that if the NVMem (nvm) is not yet registered the attribute is
there but we return -EAGAIN to the userspace.
Re: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw
On Tue, Mar 23, 2021 at 10:30:16PM +0800, lyl2...@mail.ustc.edu.cn wrote: > > > > > -原始邮件- > > 发件人: "Mika Westerberg" > > 发送时间: 2021-03-23 22:06:47 (星期二) > > 收件人: "Lv Yunlong" > > 抄送: andreas.noe...@gmail.com, michael.ja...@intel.com, > > yehezkel...@gmail.com, linux-...@vger.kernel.org, > > linux-kernel@vger.kernel.org > > 主题: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw > > > > Hi, > > > > On Mon, Mar 22, 2021 at 08:15:12PM -0700, Lv Yunlong wrote: > > > In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc() > > > with an initial reference. Before calling tb_cfg_request_sync(), > > > there is no refcount inc operation. tb_cfg_request_sync() > > > calls tb_cfg_request(..,req,..) and if the callee failed, > > > the initial reference of req is dropped and req is freed. > > > > > > Later in tb_cfg_read_raw before the err check, > > > tb_cfg_request_put(req) is called again. It may cause error > > > in race. > > > > Hmm, tb_cfg_request() does tb_cfg_request_get() too and in case of error > > it does tb_cfg_request_put(). So the refcount should be fine. What am I > > missing? > > > > > > > > My patch puts tb_cfg_request_put(req) after the err check > > > finished to avoid unexpected result. > > > > > > Signed-off-by: Lv Yunlong > > > --- > > > drivers/thunderbolt/ctl.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c > > > index f1aeaff9f368..bb60269c89ab 100644 > > > --- a/drivers/thunderbolt/ctl.c > > > +++ b/drivers/thunderbolt/ctl.c > > > @@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl > > > *ctl, void *buffer, > > > > > > res = tb_cfg_request_sync(ctl, req, timeout_msec); > > > > > > - tb_cfg_request_put(req); > > > - > > > if (res.err != -ETIMEDOUT) > > > break; > > > > > > + tb_cfg_request_put(req); > > > + > > > /* Wait a bit (arbitrary time) until we send a retry */ > > > usleep_range(10, 100); > > > } > > > -- > > > 2.25.1 > > > > > I'm very sorry, i was ashamed that i had missed the tb_cfg_request_get() in > tb_cfg_request(). It happens, no worries :)
Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw
Hi, On Mon, Mar 22, 2021 at 08:15:12PM -0700, Lv Yunlong wrote: > In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc() > with an initial reference. Before calling tb_cfg_request_sync(), > there is no refcount inc operation. tb_cfg_request_sync() > calls tb_cfg_request(..,req,..) and if the callee failed, > the initial reference of req is dropped and req is freed. > > Later in tb_cfg_read_raw before the err check, > tb_cfg_request_put(req) is called again. It may cause error > in race. Hmm, tb_cfg_request() does tb_cfg_request_get() too and in case of error it does tb_cfg_request_put(). So the refcount should be fine. What am I missing? > > My patch puts tb_cfg_request_put(req) after the err check > finished to avoid unexpected result. > > Signed-off-by: Lv Yunlong > --- > drivers/thunderbolt/ctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c > index f1aeaff9f368..bb60269c89ab 100644 > --- a/drivers/thunderbolt/ctl.c > +++ b/drivers/thunderbolt/ctl.c > @@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl > *ctl, void *buffer, > > res = tb_cfg_request_sync(ctl, req, timeout_msec); > > - tb_cfg_request_put(req); > - > if (res.err != -ETIMEDOUT) > break; > > + tb_cfg_request_put(req); > + > /* Wait a bit (arbitrary time) until we send a retry */ > usleep_range(10, 100); > } > -- > 2.25.1 >
Re: [PATCH] PCI: PM: Do not read power state in pci_enable_device_flags()
On Tue, Mar 16, 2021 at 04:51:40PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> It should not be necessary to update the current_state field of
> struct pci_dev in pci_enable_device_flags() before calling
> do_pci_enable_device() for the device, because none of the
> code between that point and the pci_set_power_state() call in
> do_pci_enable_device() invoked later depends on it.
>
> Moreover, doing that is actively harmful in some cases. For example,
> if the given PCI device depends on an ACPI power resource whose _STA
> method initially returns 0 ("off"), but the config space of the PCI
> device is accessible and the power state retrieved from the
> PCI_PM_CTRL register is D0, the current_state field in the struct
> pci_dev representing that device will get out of sync with the
> power.state of its ACPI companion object and that will lead to
> power management issues going forward.
>
> To avoid such issues it is better to leave the current_state value
> as is until it is changed to PCI_D0 by do_pci_enable_device() as
> appropriate. However, the power state of the device is not changed
> to PCI_D0 if it is already enabled when pci_enable_device_flags()
> gets called for it, so update its current_state in that case, but
> use pci_update_current_state() covering platform PM too for that.
>
> Link:
> https://lore.kernel.org/lkml/20210314000439.3138941-1-luzmaximil...@gmail.com/
> Reported-by: Maximilian Luz
> Tested-by: Maximilian Luz
> Signed-off-by: Rafael J. Wysocki
Reviewed-by: Mika Westerberg
Re: [PATCH v1 1/1] pinctrl: intel: Show the GPIO base calculation explicitly
On Mon, Mar 08, 2021 at 06:49:10PM +0200, Andy Shevchenko wrote:
> During the split of intel_pinctrl_add_padgroups(), the _by_size() variant
> missed the GPIO base calculations and hence made unable to retrieve proper
> GPIO number.
>
> Assign the gpio_base explicitly in _by_size() variant.
>
> While at it, differentiate NOMAP case with the rest in _by_gpps() variant.
>
> Fixes: 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for
> better maintenance")
> Reported-and-tested-by: Maximilian Luz
> Signed-off-by: Andy Shevchenko
I think this needs stable tag too.
Acked-by: Mika Westerberg
Re: [PATCH v1 1/1] pinctrl: intel: No need to disable IRQs in the handler
On Thu, Mar 04, 2021 at 12:54:32PM +0200, Andy Shevchenko wrote: > In IRQ handler interrupts are already disabled, hence no need > to repeat it. Even in the threaded case, which is disabled here, > it is not a problem because IRQ framework serializes descriptor > handling. Remove disabling IRQ part in the handler. > > Signed-off-by: Andy Shevchenko Acked-by: Mika Westerberg
Re: [PATCH v1 1/3] gpiolib: acpi: Add ACPI_GPIO_QUIRK_ABSOLUTE_NUMBER quirk
On Tue, Mar 02, 2021 at 05:09:24PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 02, 2021 at 03:48:43PM +0100, Linus Walleij wrote:
> > On Thu, Feb 25, 2021 at 5:33 PM Andy Shevchenko
> > wrote:
> >
> > > On some systems the ACPI tables has wrong pin number and instead of
> > > having a relative one it provides an absolute one in the global GPIO
> > > number space.
> > >
> > > Add ACPI_GPIO_QUIRK_ABSOLUTE_NUMBER quirk to cope with such cases.
> > >
> > > Fixes: ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the
> > > expanders on Galileo Gen 2")
> > > Depends-on: 0ea683931adb ("gpio: dwapb: Convert driver to using the
> > > GPIO-lib-based IRQ-chip")
> > > Signed-off-by: Andy Shevchenko
> >
> > OH THE HORROR!
> > However, we discussed it before. It is as it is.
>
> Unfortunately :-( (And recently it seems MS does something really "creative"
> on
> ARM ACPI platform)
>
> > It's the right place to fix the problem, so:
> > Acked-by: Linus Walleij
>
> I am waiting for Mika, but if he keeps silent let's say to the end of the day,
> I will submit it as is to the v5.12-rcX fixes.
Sorry for the delay - I somehow missed this. Feel free to add my ACK too.
Re: [PATCH v1] watchdog: wdat: add param. to start wdog on module insertion
Hi,
On Thu, Feb 18, 2021 at 05:32:00PM +0100, Flavio Suligoi wrote:
> Add the parameter "start_enable" to start the watchdog
> directly on module insertion.
>
> In an embedded system, for some applications, the watchdog
> must be activated as soon as possible.
>
> In some embedded x86 boards the watchdog can be activated
> directly by the BIOS (with an appropriate setting of the
> BIOS setup). In other cases, when this BIOS feature is not
> present, the possibility to start the watchdog immediately
> after the module loading can be very useful.
>
> Signed-off-by: Flavio Suligoi
> ---
> drivers/watchdog/wdat_wdt.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index cec7917790e5..b990d0197d2e 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -61,6 +61,12 @@ module_param(timeout, int, 0);
> MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
>__MODULE_STRING(WDAT_DEFAULT_TIMEOUT) ")");
>
> +#define START_DEFAULT0
> +static int start_enabled = START_DEFAULT;
> +module_param(start_enabled, int, 0);
> +MODULE_PARM_DESC(start_enabled, "Watchdog is started on module insertion "
> + "(default=" __MODULE_STRING(START_DEFAULT) ")");
> +
> static int wdat_wdt_read(struct wdat_wdt *wdat,
>const struct wdat_instruction *instr, u32 *value)
> {
> @@ -437,6 +443,8 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> }
>
> wdat_wdt_boot_status(wdat);
> + if (start_enabled)
> + wdat_wdt_start(>wdd);
No objections to this if it is really needed. However, I think it is
better start the watchdog after devm_watchdog_register_device() has been
called so we have everything initialized.
> wdat_wdt_set_running(wdat);
>
> ret = wdat_wdt_enable_reboot(wdat);
> --
> 2.25.1
Re: [PATCH] watchdog: wdat_wdg: fix typo
On Tue, Feb 16, 2021 at 03:17:27PM +0100, Flavio Suligoi wrote: > Fix the following typo: > > "recommeded" --> "recommended" > "firmare"--> "firmware" > > Signed-off-by: Flavio Suligoi Reviewed-by: Mika Westerberg
Re: [PATCH] ACPI: property: Fix fwnode string properties matching
On Thu, Feb 11, 2021 at 07:30:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> Property matching does not work for ACPI fwnodes if the value of the
> given property is not represented as a package in the _DSD package
> containing it. For example, the "compatible" property in the _DSD
> below
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
> }
> })
>
> will not be found by fwnode_property_match_string(), because the ACPI
> code handling device properties does not regard the single value as a
> "list" in that case.
>
> Namely, fwnode_property_match_string() invoked to match a given
> string property value first calls fwnode_property_read_string_array()
> with the last two arguments equal to NULL and 0, respectively, in
> order to count the items in the value of the given property, with the
> assumption that this value may be an array. For ACPI fwnodes, that
> operation is carried out by acpi_node_prop_read() which calls
> acpi_data_prop_read() for this purpose. However, when the return
> (val) pointer is NULL, that function only looks for a property whose
> value is a package without checking the single-value case at all.
>
> To fix that, make acpi_data_prop_read() check the single-value case
> regardless of the return pointer value if its return pointer argument
> is NULL and modify acpi_data_prop_read_single() handling that case to
> attempt to read the value of the property if the return pointer is
> NULL and return 1 if that succeeds.
>
> Fixes: 3708184afc77 ("device property: Move FW type specific functionality to
> FW specific files")
> Reported-by: Calvin Johnson
> Cc: 4.13+ # 4.13+
> Signed-off-by: Rafael J. Wysocki
Reviewed-by: Mika Westerberg
Re: [v4] PCI: Avoid unsync of LTR mechanism configuration
On Thu, Feb 04, 2021 at 05:51:25PM +0800, mingchuang.q...@mediatek.com wrote: > From: Mingchuang Qiao > > In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is > configured in pci_configure_ltr(). If device and bridge both support LTR > mechanism, the "LTR Mechanism Enable" bit of device and bridge will be > enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1. > > If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit > of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However, > the pci_dev->ltr_path value of bridge is still 1. > > For following conditions, check and re-configure "LTR Mechanism Enable" bit > of bridge to make "LTR Mechanism Enable" bit match ltr_path value. >-before configuring device's LTR for hot-remove/hot-add >-before restoring device's DEVCTL2 register when restore device state > > Signed-off-by: Mingchuang Qiao Reviewed-by: Mika Westerberg
Re: [v3] PCI: Avoid unsync of LTR mechanism configuration
On Wed, Feb 03, 2021 at 10:14:01AM +0800, Mingchuang Qiao wrote: > Hi, > > On Mon, 2021-02-01 at 13:32 +0200, Mika Westerberg wrote: > > Hi, > > > > On Fri, Jan 29, 2021 at 03:11:37PM +0800, mingchuang.q...@mediatek.com > > wrote: > > > From: Mingchuang Qiao > > > > > > In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is > > > configured in pci_configure_ltr(). If device and bridge both support LTR > > > mechanism, the "LTR Mechanism Enable" bit of device and bridge will be > > > enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1. > > > > > > If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit > > > of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However, > > > the pci_dev->ltr_path value of bridge is still 1. > > > > > > For following conditions, check and re-configure "LTR Mechanism Enable" > > > bit > > > of bridge to make "LTR Mechanism Enable" bit mtach ltr_path value. > > > > Typo mtach -> match. > > > > >-before configuring device's LTR for hot-remove/hot-add > > >-before restoring device's DEVCTL2 register when restore device state > > > > > > Signed-off-by: Mingchuang Qiao > > > --- > > > changes of v2 > > > -modify patch description > > > -reconfigure bridge's LTR before restoring device DEVCTL2 register > > > changes of v3 > > > -call pci_reconfigure_bridge_ltr() in probe.c > > > > Hmm, which part of this patch takes care of the reset path? It is not > > entirely clear to me at least. > > > > When device resets and link goes down, there seems to have two methods > to recover for software. >-One is that trigger device removal and rescan. >-The other is that restore device with pci_restore_state() after link > comes back up. > For above both scenarios, we need check and reconfigure "LTR Mechanism > Enable" bit of bridge. It's also this patch intends to do. >-For the rescan scenario, it's done in pci_configure_ltr(). >-For the restore scenario, it's done in pci_restore_pcie_state(). Okay, thanks for the clarification!
Re: [v3] PCI: Avoid unsync of LTR mechanism configuration
Hi,
On Fri, Jan 29, 2021 at 03:11:37PM +0800, mingchuang.q...@mediatek.com wrote:
> From: Mingchuang Qiao
>
> In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is
> configured in pci_configure_ltr(). If device and bridge both support LTR
> mechanism, the "LTR Mechanism Enable" bit of device and bridge will be
> enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1.
>
> If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit
> of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However,
> the pci_dev->ltr_path value of bridge is still 1.
>
> For following conditions, check and re-configure "LTR Mechanism Enable" bit
> of bridge to make "LTR Mechanism Enable" bit mtach ltr_path value.
Typo mtach -> match.
>-before configuring device's LTR for hot-remove/hot-add
>-before restoring device's DEVCTL2 register when restore device state
>
> Signed-off-by: Mingchuang Qiao
> ---
> changes of v2
> -modify patch description
> -reconfigure bridge's LTR before restoring device DEVCTL2 register
> changes of v3
> -call pci_reconfigure_bridge_ltr() in probe.c
Hmm, which part of this patch takes care of the reset path? It is not
entirely clear to me at least.
> ---
> drivers/pci/pci.c | 25 +
> drivers/pci/pci.h | 1 +
> drivers/pci/probe.c | 13 ++---
> 3 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b9fecc25d213..12b557c8f062 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1437,6 +1437,24 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> return 0;
> }
>
> +void pci_reconfigure_bridge_ltr(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCIEASPM
> + struct pci_dev *bridge;
> + u32 ctl;
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, );
> + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> + pci_dbg(bridge, "re-enabling LTR\n");
> + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + }
> + }
> +#endif
> +}
> +
> static void pci_restore_pcie_state(struct pci_dev *dev)
> {
> int i = 0;
> @@ -1447,6 +1465,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> if (!save_state)
> return;
>
> + /*
> + * Downstream ports reset the LTR enable bit when link goes down.
> + * Check and re-configure the bit here before restoring device.
> + * PCIe r5.0, sec 7.5.3.16.
> + */
> + pci_reconfigure_bridge_ltr(dev);
> +
> cap = (u16 *)_state->cap.data[0];
> pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
> pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5c59365092fa..a660a01358c5 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -111,6 +111,7 @@ void pci_free_cap_save_buffers(struct pci_dev *dev);
> bool pci_bridge_d3_possible(struct pci_dev *dev);
> void pci_bridge_d3_update(struct pci_dev *dev);
> void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev);
> +void pci_reconfigure_bridge_ltr(struct pci_dev *dev);
Nit: calling it pci_bridge_reconfigure_ltr() would match better with the
other function names.
>
> static inline void pci_wakeup_event(struct pci_dev *dev)
> {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..fa6075093f3b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2132,9 +2132,16 @@ static void pci_configure_ltr(struct pci_dev *dev)
>* Complex and all intermediate Switches indicate support for LTR.
>* PCIe r4.0, sec 6.18.
>*/
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> - ((bridge = pci_upstream_bridge(dev)) &&
> - bridge->ltr_path)) {
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + dev->ltr_path = 1;
> + return;
> + }
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pci_reconfigure_bridge_ltr(dev);
> pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
>PCI_EXP_DEVCTL2_LTR_EN);
> dev->ltr_path = 1;
> --
> 2.18.0
Re: [v2] PCI: Avoid unsync of LTR mechanism configuration
Hi,
On Thu, Jan 28, 2021 at 06:05:31PM +0800, mingchuang.q...@mediatek.com wrote:
> From: Mingchuang Qiao
>
> In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is
> configured in pci_configure_ltr(). If device and bridge both support LTR
> mechanism, the "LTR Mechanism Enable" bit of device and bridge will be
> enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1.
>
> If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit
> of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However,
> the pci_dev->ltr_path value of bridge is still 1.
>
> For following conditions, check and re-configure "LTR Mechanism Enable" bit
> of bridge to make "LTR Mechanism Enable" bit mtach ltr_path value.
>-before configuring device's LTR for hot-remove/hot-add
>-before restoring device's DEVCTL2 register when restore device state
>
> Signed-off-by: Mingchuang Qiao
> ---
> changes of v2
> -modify patch description
> -reconfigure bridge's LTR before restoring device DEVCTL2 register
> ---
> drivers/pci/pci.c | 25 +
> drivers/pci/probe.c | 19 ---
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b9fecc25d213..88b4eb70c252 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1437,6 +1437,24 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> return 0;
> }
>
> +static void pci_reconfigure_bridge_ltr(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCIEASPM
> + struct pci_dev *bridge;
> + u32 ctl;
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, );
> + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> + pci_dbg(bridge, "re-enabling LTR\n");
> + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + }
> + }
> +#endif
> +}
> +
> static void pci_restore_pcie_state(struct pci_dev *dev)
> {
> int i = 0;
> @@ -1447,6 +1465,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> if (!save_state)
> return;
>
> + /*
> + * Downstream ports reset the LTR enable bit when link goes down.
> + * Check and re-configure the bit here before restoring device.
> + * PCIe r5.0, sec 7.5.3.16.
> + */
> + pci_reconfigure_bridge_ltr(dev);
> +
> cap = (u16 *)_state->cap.data[0];
> pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
> pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..4ad172517fd2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2132,9 +2132,22 @@ static void pci_configure_ltr(struct pci_dev *dev)
>* Complex and all intermediate Switches indicate support for LTR.
>* PCIe r4.0, sec 6.18.
>*/
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> - ((bridge = pci_upstream_bridge(dev)) &&
> - bridge->ltr_path)) {
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + dev->ltr_path = 1;
> + return;
> + }
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, );
> + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> + pci_dbg(bridge, "re-enabling LTR\n");
> + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + }
> +
Can't you use pci_reconfigure_bridge_ltr() here too?
Otherwise looks good.
Re: [PATCH 00/12] Rid W=1 warnings from Thunderbolt
Hi Lee,
On Wed, Jan 27, 2021 at 11:25:42AM +, Lee Jones wrote:
> This set is part of a larger effort attempting to clean-up W=1
> kernel builds, which are currently overwhelmingly riddled with
> niggly little warnings.
>
> Only 1 small set required for Thunderbolt. Pretty good!
>
> Lee Jones (12):
> thunderbolt: dma_port: Remove unused variable 'ret'
> thunderbolt: cap: Fix kernel-doc formatting issue
> thunderbolt: ctl: Demote non-conformant kernel-doc headers
> thunderbolt: eeprom: Demote non-conformant kernel-doc headers to
> standard comment blocks
> thunderbolt: pa: Demote non-conformant kernel-doc headers
> thunderbolt: xdomain: Fix 'tb_unregister_service_driver()'s 'drv'
> param
> thunderbolt: nhi: Demote some non-conformant kernel-doc headers
> thunderbolt: tb: Kernel-doc function headers should document their
> parameters
> thunderbolt: swit: Demote a bunch of non-conformant kernel-doc headers
> thunderbolt: icm: Fix a couple of formatting issues
> thunderbolt: tunnel: Fix misspelling of 'receive_path'
> thunderbolt: swit: Fix function name in the header
I applied all of the changes that touch static functions. For non-static
functions I will send a patch set shortly that adds the missing bits for
the kernel-doc descriptions. I also fixed $subject lines of few patches
("switch:" instead of "swit:").
Please check that I got everything correct in
git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
Thanks!
Re: [PATCH V2 01/12] thunderbolt: dma_port: Check 'dma_port_flash_write_block()'s return value
On Thu, Jan 28, 2021 at 08:52:33AM +, Lee Jones wrote: > ... and take the error path if it fails. > > Fixes the following W=1 kernel build warning(s): > > drivers/thunderbolt/dma_port.c: In function ‘dma_port_flash_write_block’: > drivers/thunderbolt/dma_port.c:331:6: warning: variable ‘ret’ set but not > used [-Wunused-but-set-variable] > > Cc: Andreas Noever > Cc: Michael Jamet > Cc: Mika Westerberg > Cc: Yehezkel Bernat > Cc: linux-...@vger.kernel.org > Signed-off-by: Lee Jones Applied, thanks!
Re: [PATCH 05/12] thunderbolt: pa: Demote non-conformant kernel-doc headers
On Thu, Jan 28, 2021 at 08:23:30AM +, Lee Jones wrote: > On Wed, 27 Jan 2021, Mika Westerberg wrote: > > > On Wed, Jan 27, 2021 at 04:13:20PM +, Lee Jones wrote: > > > On Wed, 27 Jan 2021, Andy Shevchenko wrote: > > > > > > > On Wednesday, January 27, 2021, Lee Jones wrote: > > > > > > > > > Fixes the following W=1 kernel build warning(s): > > > > > > > > > > drivers/thunderbolt/path.c:476: warning: Function parameter or member > > > > > 'path' not described in 'tb_path_activate' > > > > > drivers/thunderbolt/path.c:568: warning: Function parameter or member > > > > > 'path' not described in 'tb_path_is_invalid' > > > > > > > > > > > > > > I think the intention was to describe them in kernel doc format, perhaps > > > > you need to add descriptions of the fields? > > > > > > For changes like this, I've been working to the following rule: > > > > > > - I'll provide fix-ups; if and only if the author has had a > > > reasonable attempt at providing a conformant kernel-doc header. > > > > > > So if the headers are just suffering from a little doc-rot i.e. the > > > API has changed, but the doc update was omitted, or most of the > > > parameters/members are documented, but some were forgotten about etc, > > > or if there are formatting issues, I'll happily take up the slack and > > > polish those up a bit. > > > > > > However, if no attempt was made, then they get demoted. > > > > > > I don't want to get into a situation where authors delicately provide > > > weak documentation with the expectation that someone else will come > > > along and turn them into conformant docs. > > > > > > If authors wish to come back, provide proper descriptions & > > > formatting and subsequently re-promote them again, then all power to > > > them. > > > > Thanks for pointing these out. I prefer we fix the kernel-docs (add what > > is missing) instead. I'll take care of that. > > Are you planning on actually using this? Yes, eventually :) > I don't see a Doc link for these functions in Mainline: > > `git grep kernel-doc:: | grep thunderbolt` There is not one now but I would like to have the kernel-docs at least in correct format so we can add the link later.
Re: [PATCH 05/12] thunderbolt: pa: Demote non-conformant kernel-doc headers
On Wed, Jan 27, 2021 at 04:13:20PM +, Lee Jones wrote: > On Wed, 27 Jan 2021, Andy Shevchenko wrote: > > > On Wednesday, January 27, 2021, Lee Jones wrote: > > > > > Fixes the following W=1 kernel build warning(s): > > > > > > drivers/thunderbolt/path.c:476: warning: Function parameter or member > > > 'path' not described in 'tb_path_activate' > > > drivers/thunderbolt/path.c:568: warning: Function parameter or member > > > 'path' not described in 'tb_path_is_invalid' > > > > > > > > I think the intention was to describe them in kernel doc format, perhaps > > you need to add descriptions of the fields? > > For changes like this, I've been working to the following rule: > > - I'll provide fix-ups; if and only if the author has had a > reasonable attempt at providing a conformant kernel-doc header. > > So if the headers are just suffering from a little doc-rot i.e. the > API has changed, but the doc update was omitted, or most of the > parameters/members are documented, but some were forgotten about etc, > or if there are formatting issues, I'll happily take up the slack and > polish those up a bit. > > However, if no attempt was made, then they get demoted. > > I don't want to get into a situation where authors delicately provide > weak documentation with the expectation that someone else will come > along and turn them into conformant docs. > > If authors wish to come back, provide proper descriptions & > formatting and subsequently re-promote them again, then all power to > them. Thanks for pointing these out. I prefer we fix the kernel-docs (add what is missing) instead. I'll take care of that.
Re: [PATCH 01/12] thunderbolt: dma_port: Remove unused variable 'ret'
Hi, On Wed, Jan 27, 2021 at 04:19:16PM +, Lee Jones wrote: > On Wed, 27 Jan 2021, Andy Shevchenko wrote: > > > On Wednesday, January 27, 2021, Lee Jones wrote: > > > > > Fixes the following W=1 kernel build warning(s): > > > > > > drivers/thunderbolt/dma_port.c: In function ‘dma_port_flash_write_block’: > > > drivers/thunderbolt/dma_port.c:331:6: warning: variable ‘ret’ set but > > > not used [-Wunused-but-set-variable] > > > > > > > > Is it scripted somehow? > > A script opens up the file on the warning line. > > The patch is hand-written. > > > Because I am not sure we are okay to simply drop the assignment. > > I've been careful not to change the semantics of the code. > > The return value has never been checked since the driver's inception 4 > years ago. > > However, if this is an oversight and the intention was to check the > value and error-out during a failure condition, I can make that > happen. > > I would need a nod from the author before I make such a change. It's actually an oversight from my side. It should do something like: if (ret) return ret: there. Feel free to fix it up :) Thanks!
Re: [RFC 3/3] thunderbolt: build kunit tests without structleak plugin
Hi Arnd, On Mon, Jan 25, 2021 at 01:45:28PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > The structleak plugin causes the stack frame size to grow immensely: > > drivers/thunderbolt/test.c:1529:1: error: the frame size of 1176 bytes is > larger than 1024 bytes [-Werror=frame-larger-than=] > > Turn it off in this file. > > Signed-off-by: Arnd Bergmann To me this is a reasonable work around so I can pick this up to Thunderbolt tree if no objections. Thanks BTW, for doing this. I got a report from buildbot some time ago about the this but did not have time to figure out how to fix it :)
Re: [PATCH] ACPI / device_sysfs: Prefer "compatible" modalias
On Fri, Jan 22, 2021 at 08:53:02PM +0800, Kai-Heng Feng wrote:
> Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
> "compatible" is present") may create two "MODALIAS=" in uevent file if
> conditions are met.
>
> This breaks systemd-udevd, which assumes each "key" in uevent file is
> unique. The internal implementation of systemd-udevd overwrites the
> first MODALIAS with the second one, so its kmod rule doesn't load driver
> for the first MODALIAS.
>
> So if both ACPI modalias and OF modalias are present, use the latter
> one to ensure there's only one MODALIAS.
>
> Reference: https://github.com/systemd/systemd/pull/18163
> Cc: AceLan Kao
> Cc: "Rafael J. Wysocki"
> Cc: Greg Kroah-Hartman
> Cc: Andy Shevchenko
> Suggested-by: Mika Westerberg
> Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible"
> is present")
> Signed-off-by: Kai-Heng Feng
Reviewed-by: Mika Westerberg
Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
Hi, On Fri, Jan 22, 2021 at 03:03:11PM +0800, Mingchuang Qiao wrote: > On Thu, 2021-01-21 at 16:31 -0600, Bjorn Helgaas wrote: > > [+cc Alex and Mingchuang et al from > > https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.q...@mediatek.com] > > > > On Tue, Jan 19, 2021 at 04:14:10PM +0300, Mika Westerberg wrote: > > > PCIe r5.0, sec 7.5.3.16 says that the downstream ports must reset the > > > LTR enable bit if the link goes down (port goes DL_Down status). Now, if > > > we had LTR previously enabled and the PCIe endpoint gets hot-removed and > > > then hot-added back the ->ltr_path of the downstream port is still set > > > but the port now does not have the LTR enable bit set anymore. > > > > > > For this reason check if the bridge upstream had LTR enabled previously > > > and re-enable it before enabling LTR for the endpoint. > > > > > > Reported-by: Utkarsh H Patel > > > Signed-off-by: Mika Westerberg > > > > I think this and Mingchuang's patch, which is essentially identical, > > are right and solves the problem for hot-remove/hot-add. In that > > scenario we call pci_configure_ltr() on the hot-added device, and > > with this patch, we'll re-enable LTR on the bridge leading to the new > > device before enabling LTR on the new device itself. > > > > But don't we have a similar problem if we simply do a Fundamental > > Reset on a device? I think the reset path will restore the device's > > state, including PCI_EXP_DEVCTL2, but it doesn't do anything with the > > upstream bridge, does it? > > > > Yes. I think the same problem exists under such scenario, and that’s the > issue my patch intends to resolve. > I also prepared a v2 patch for review(update the patch description). > Shall I submit the v2 patch for review? I looked at your patch and indeed it is essentially doing the same as this one. So let's forget this patch and go forward with yours :) Would you like to expand your patch to handle the reset case too that Bjorn desribes below? > > So if a bridge and a device below it both have LTR enabled, can't we > > have the following: > > > > - bridge LTR enabled > > - device LTR enabled > > - reset device, e.g., via Secondary Bus Reset > > - link goes down, bridge disables LTR > > - link comes back up, LTR disabled in both bridge and device > > - restore device state, including LTR enable > > - device sends LTR message > > - bridge reports Unsupported Request
Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias
On Thu, Jan 21, 2021 at 02:22:43PM +0800, Kai-Heng Feng wrote: > On Tue, Jan 19, 2021 at 6:34 PM Greg Kroah-Hartman > wrote: > > > > On Tue, Jan 19, 2021 at 11:41:59AM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > > > > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > > > > wrote: > > > > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > > > > > ... > > > > > > > > Who will use OF_MODALIAS and where have you documented it? > > > > > > > > After this lands in mainline, I'll modify the pull request for systemd > > > > to add a new rule for OF_MODALIAS. > > > > I'll modify the comment on the function to document the change. > > > > > > I'm wondering why to have two fixes in two places instead of fixing udev > > > to > > > understand multiple MODALIAS= events? > > > > It's not a matter of multiple events, it's a single event with a > > key/value pair with duplicate keys and different values. > > > > What is this event with different values supposed to be doing in > > userspace? Do you want multiple invocations of `modprobe` or something > > else? > > > > Usually a "device" only has a single "signature" that modprobe uses to > > look up the correct module for. Modules can support any number of > > device signatures, but traditionally it is odd to think that a device > > itself can be supported by multiple modules, which is what you are > > saying is happening here. > > > > So what should userspace do with this, and why does a device need to > > have multiple module alias signatures? > > >From the original use case [1], I think the "compatible" modalias > should be enough. > Andy and Mika, what do you think? Can we remove the ACPI modalias for this > case? Yes, I think that should work. After all we want the match to happen through the DT compatible string if the property is present, not through ACPI IDs.
Re: Multiple MODALIAS= in uevent file confuses userspace
On Mon, Jan 18, 2021 at 03:26:28PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 18, 2021 at 04:12:38PM +0200, Mika Westerberg wrote:
> > Hi,
> >
> > On Mon, Jan 18, 2021 at 02:50:33PM +0100, Rafael J. Wysocki wrote:
> > > CC Mika and Andy.
> > >
> > > On Mon, Jan 18, 2021 at 8:27 AM Kai-Heng Feng
> > > wrote:
> > > >
> > > > On Sat, Jan 9, 2021 at 12:25 AM Kai-Heng Feng
> > > > wrote:
> > > > >
> > > > > Commit 8765c5ba19490 ("ACPI / scan: Rework modalias creation when
> > > > > "compatible" is present") creates two modaliases for certain ACPI
> > > > > devices. However userspace (systemd-udevd in this case) assumes uevent
> > > > > file doesn't have duplicated keys, so two "MODALIAS=" breaks the
> > > > > assumption.
> > > > >
> > > > > Based on the assumption, systemd-udevd internally uses hashmap to
> > > > > store each line of uevent file, so the second modalias always replaces
> > > > > the first modalias.
> > > > >
> > > > > My attempt [1] is to add a new key, "MODALIAS1" for the second
> > > > > modalias. This brings up the question of whether each key in uevent
> > > > > file is unique. If it's no unique, this may break may userspace.
> > > >
> > > > Does anyone know if there's any user of the second modalias?
> > > > If there's no user of the second one, can we change it to OF_MODALIAS
> > > > or COMPAT_MODALIAS?
> >
> > The only users I'm aware are udev and the busybox equivalent (udev,
> > mdev) but I'm not sure if they use the second second modalias at all so
> > OF_MODALIAS for the DT compatible string sounds like a good way to solve
> > this.
>
> As udev seems to "break" with this (which is where we got the original
> report from), I don't think you need to worry about that user :)
Ah right - it is the same udev used in busybox too :)
> Does anyone use mdev anymore, and in any ACPI-supported systems?
My guess is that some "embedded" distros such as Yocto and Buildroot
(Gentoo perhaps) may still use it, and at least Yocto is being used in
ACPI enabled systems.
Re: Multiple MODALIAS= in uevent file confuses userspace
Hi,
On Mon, Jan 18, 2021 at 02:50:33PM +0100, Rafael J. Wysocki wrote:
> CC Mika and Andy.
>
> On Mon, Jan 18, 2021 at 8:27 AM Kai-Heng Feng
> wrote:
> >
> > On Sat, Jan 9, 2021 at 12:25 AM Kai-Heng Feng
> > wrote:
> > >
> > > Commit 8765c5ba19490 ("ACPI / scan: Rework modalias creation when
> > > "compatible" is present") creates two modaliases for certain ACPI
> > > devices. However userspace (systemd-udevd in this case) assumes uevent
> > > file doesn't have duplicated keys, so two "MODALIAS=" breaks the
> > > assumption.
> > >
> > > Based on the assumption, systemd-udevd internally uses hashmap to
> > > store each line of uevent file, so the second modalias always replaces
> > > the first modalias.
> > >
> > > My attempt [1] is to add a new key, "MODALIAS1" for the second
> > > modalias. This brings up the question of whether each key in uevent
> > > file is unique. If it's no unique, this may break may userspace.
> >
> > Does anyone know if there's any user of the second modalias?
> > If there's no user of the second one, can we change it to OF_MODALIAS
> > or COMPAT_MODALIAS?
The only users I'm aware are udev and the busybox equivalent (udev,
mdev) but I'm not sure if they use the second second modalias at all so
OF_MODALIAS for the DT compatible string sounds like a good way to solve
this.
Re: [PATCH] thunderbolt: Constify static attribute_group structs
On Sat, Jan 09, 2021 at 12:09:19AM +0100, Rikard Falkeborn wrote: > The only usage of these is to put their addresses in arrays of pointers > to const attribute_groups. Make them const to allow the compiler to put > them in read-only memory. > > Signed-off-by: Rikard Falkeborn Applied to thunderbolt.git/next, thanks!
Re: [RFT][PATCH v1 0/3] ACPI: scan: Defer enumeration of devices with significant dependencies
Hi Rafael, On Mon, Dec 14, 2020 at 09:23:47PM +0100, Rafael J. Wysocki wrote: > Hi, > > This series addresses some enumeration ordering issues by using information > from _DEP to defer the enumeration of devices that are likely to depend on > operation region (OpRegion) handlers supplied by the drivers of other > devices. > > This allows the OpRegion suppliers to be probed and start working before the > devices depending on them are enumerated. For the whole series, Reviewed-by: Mika Westerberg
Re: [PATCH net-next] net: thunderbolt: convert comma to semicolon
On Wed, Dec 09, 2020 at 09:38:52PM +0800, Zheng Yongjun wrote: > Replace a comma between expression statements by a semicolon. > > Signed-off-by: Zheng Yongjun Acked-by: Mika Westerberg
Re: linux-next: manual merge of the drm tree with the pci tree
Hi,
On Tue, Dec 08, 2020 at 01:27:54PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the drm tree got a conflict in:
>
> drivers/gpu/vga/vga_switcheroo.c
>
> between commit:
>
> 99efde6c9bb7 ("PCI/PM: Rename pci_wakeup_bus() to pci_resume_bus()")
>
> from the pci tree and commit:
>
> 9572e6693cd7 ("vga_switcheroo: simplify the return expression of
> vga_switcheroo_runtime_resume")
>
> from the drm tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
Thanks for the fix Stephen! Looks correct to me.
Re: [PATCH 00/12] thunderbolt: USB4 NVM upgrade improvements & Maple Ridge support
On Thu, Nov 19, 2020 at 06:55:11PM +0300, Mika Westerberg wrote: > Hi all, > > This series improves the USB4 router NVM upgrade functionality and adds > support for USB4 router operations proxy implemented by recent Intel > Thunderbolt firmware connection manager. The last patch adds support for > Intel Maple Ridge that is the first discrete Thunderbolt/USB4 controller > from Intel. > > This also includes a couple of minor cleanups and improvements around > debug logging. > > Mika Westerberg (12): > thunderbolt: Move max_boot_acl field to correct place in struct icm > thunderbolt: Log which connection manager implementation is used > thunderbolt: Log adapter numbers in decimal in path activation/deactivation > thunderbolt: Keep the parent runtime resumed for a while on device > disconnect > thunderbolt: Return -ENOTCONN when ERR_CONN is received > thunderbolt: Perform USB4 router NVM upgrade in two phases > thunderbolt: Pass metadata directly to usb4_switch_op() > thunderbolt: Pass TX and RX data directly to usb4_switch_op() > thunderbolt: Add connection manager specific hooks for USB4 router > operations > thunderbolt: Move constants for USB4 router operations to tb_regs.h > thunderbolt: Add USB4 router operation proxy for firmware connection manager > thunderbolt: Add support for Intel Maple Ridge All applied to thunderbolt.git/next.
Re: [PATCH v1 2/2] PM: ACPI: Refresh wakeup device power configuration every time
On Tue, Nov 24, 2020 at 08:46:38PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > When wakeup signaling is enabled for a bridge for the second (or every > next) time in a row, its existing device wakeup power configuration > may not match the new conditions. For example, some devices below > it may have been put into low-power states and that changes the > device wakeup power conditions or similar. This causes functional > problems to appear on some systems (for example, because of it the > Thunderbolt port on Dell Precision 5550 cannot detect devices plugged > in after it has been suspended). > > For this reason, modify __acpi_device_wakeup_enable() to refresh the > device wakeup power configuration of the target device on every > invocation, not just when it is called for that device first time > in a row. > > Signed-off-by: Rafael J. Wysocki > Reported-by: Kai-Heng Feng > Tested-by: Kai-Heng Feng Reviewed-by: Mika Westerberg
Re: [PATCH v1 1/2] PM: ACPI: PCI: Drop acpi_pm_set_bridge_wakeup()
On Tue, Nov 24, 2020 at 08:44:00PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > The idea behind acpi_pm_set_bridge_wakeup() was to allow bridges to > be reference counted for wakeup enabling, because they may be enabled > to signal wakeup on behalf of their subordinate devices and that > may happen for multiple times in a row, whereas for the other devices > it only makes sense to enable wakeup signaling once. > > However, this becomes problematic if the bridge itself is suspended, > because it is treated as a "regular" device in that case and the > reference counting doesn't work. > > For instance, suppose that there are two devices below a bridge and > they both can signal wakeup. Every time one of them is suspended, > wakeup signaling is enabled for the bridge, so when they both have > been suspended, the bridge's wakeup reference counter value is 2. > > Say that the bridge is suspended subsequently and acpi_pci_wakeup() > is called for it. Because the bridge can signal wakeup, that > function will invoke acpi_pm_set_device_wakeup() to configure it > and __acpi_pm_set_device_wakeup() will be called with the last > argument equal to 1. This causes __acpi_device_wakeup_enable() > invoked by it to omit the reference counting, because the reference > counter of the target device (the bridge) is 2 at that time. > > Now say that the bridge resumes and one of the device below it > resumes too, so the bridge's reference counter becomes 0 and > wakeup signaling is disabled for it, but there is still the other > suspended device which may need the bridge to signal wakeup on its > behalf and that is not going to work. > > To address this scenario, use wakeup enable reference counting for > all devices, not just for bridges, so drop the last argument from > __acpi_device_wakeup_enable() and __acpi_pm_set_device_wakeup(), > which causes acpi_pm_set_device_wakeup() and > acpi_pm_set_bridge_wakeup() to become identical, so drop the latter > and use the former instead of it everywhere. > > Signed-off-by: Rafael J. Wysocki Reviewed-by: Mika Westerberg
Re: How to enable auto-suspend by default
On Mon, Nov 23, 2020 at 02:54:19PM +0100, Hans de Goede wrote: > Hi, > > On 11/11/20 3:31 PM, Mika Westerberg wrote: > > On Wed, Nov 11, 2020 at 12:27:32PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 11/10/20 6:25 PM, Mika Westerberg wrote: > >>> On Tue, Nov 10, 2020 at 04:02:33PM +, Limonciello, Mario wrote: > >>>>> > >>>>> On Tue, Nov 10, 2020 at 11:57:07AM +0100, Bastien Nocera wrote: > >>>>>> Hey, > >>>>>> > >>>>>> systemd has been shipping this script to enable auto-suspend on a > >>>>>> number of USB and PCI devices: > >>>>>> > >>>>> https://github.com/systemd/systemd/blob/master/tools/chromiumos/gen_autosuspen > >>>>> d_rules.py > >>>>>> > >>>>>> The problem here is twofold. First, the list of devices is updated from > >>>>>> ChromeOS, and the original list obviously won't be updated by ChromeOS > >>>>>> developers unless a device listed exists in a ChromeBook computer, > >>>>>> which means a number of devices that do support autosuspend aren't > >>>>>> listed. > >>>>>> > >>>>>> The other problem is that this list needs to exist at all, and that it > >>>>>> doesn't seem possible for device driver developers (at various levels > >>>>>> of the stack) to opt-in to auto-suspend when all the variants of the > >>>>>> device (or at least detectable ones) support auto-suspend. > >>>>> > >>>>> A driver can say they support autosuspend today, but I think you are > >>>>> concerned about the devices that are controlled by class-compliant > >>>>> drivers, right? And for those, no, we can't do this in the kernel as > >>>>> there are just too many broken devices out there. > >>>>> > >>>> > >>>> I guess what Bastien is getting at is for newer devices supported by > >>>> class > >>>> drivers rather than having to store an allowlist in udev rules, can we > >>>> set > >>>> the allowlist in the kernel instead. Then distributions that either > >>>> don't > >>>> use systemd or don't regularly update udev rules from systemd can take > >>>> advantage of better defaults on modern hardware. > >>>> > >>>> The one item that stood out to me in that rules file was 8086:a0ed. > >>>> It's listed as "Volteer XHCI", but that same device ID is actually > >>>> present > >>>> in an XPS 9310 in front of me as well and used by the xhci-pci kernel > >>>> module. > >>>> > >>>> Given we're effectively ending up with the combination of runtime PM > >>>> turned > >>>> on by udev rules, do we need something like this for that ID: > >>>> > >>>> https://github.com/torvalds/linux/commit/6a7c533d4a1854f54901a065d8c672e890400d8a > >>>> > >>>> @Mika Westerberg should 8086:a0ed be quirked like the TCSS xHCI too? > >>> > >>> I think this one is the TGL PCH xHCI. The quirk currently for xHCI > >>> controllers that are part of the TCSS (Type-C SubSystem) where it is > >>> important to put all devices into low power mode whenever possible, > >>> otherwise it keeps the whole block on. > >> > >> Note that there are currently some IDs missing from the xHCIs which > >> are part of the TCSS too. At least the id for the xHCI in the thunderbolt > >> controller on the Lenovo T14 gen 1 is missing. I started a discussion > >> about extending the kernel quirk list for this vs switching to hwdb > >> a while a go: > >> > >> https://lore.kernel.org/linux-usb/b8b21ba3-0a8a-ff54-5e12-cf8960651...@redhat.com/ > >> > >> The conclusion back then was to switch to hwdb, but I never got around to > >> this. > > > > The reason I've added these to the xHCI driver is that it works even if > > you are running some really small userspace (like busybox). Also for the > > xHCI in TCSS we know for sure that it fully supports D3cold. > > > > (The one you refer above is actually mistake from my side as I never > > tested Alpine Ridge LP controller which I think this is). > > Ok, so I'll submit a patch adding the 15c1 product-id for the > INTEL_ALPINE_RIDGE_LP_2C_XHCI controller to the list of ids for which we > set the XHCI_DEFAULT_PM_RUNTIME_ALLOW quirk. To fix the much too high > idle-power consumption problem on devices with this Alpine Ridge variant. Thanks!
Re: [PATCH v2] docs: ACPI: enumeration: add PCI hierarchy representation
On Fri, Nov 20, 2020 at 12:11:25PM +0100, Flavio Suligoi wrote: > For "fixed" PCI devices, such as chips directly soldered > on the main board (ethernet, Wi-Fi, serial ports, etc.), > it is possible to find an ACPI enumeration. > > This allows to add useful properties to these devices. > Just for an example: the property "gpio-line-names" can be > added to the pins of a GPIO expander on the PCI bus. > > In order to find the ACPI name of a PCI device, it's necessary > to disassemble the BIOS ACPI tables (in particular the DSDT) > and also to analyze the PCI bus topology of the board. > > This patch, with a practical example, show how to do this. > > Signed-off-by: Flavio Suligoi Reviewed-by: Mika Westerberg
[PATCH 03/12] thunderbolt: Log adapter numbers in decimal in path activation/deactivation
This makes it consistent with other debug logs that already are using decimal number for adapters (ports). Signed-off-by: Mika Westerberg --- drivers/thunderbolt/path.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c index 7c2c45d9ba4a..ca7d738d66de 100644 --- a/drivers/thunderbolt/path.c +++ b/drivers/thunderbolt/path.c @@ -454,7 +454,7 @@ void tb_path_deactivate(struct tb_path *path) return; } tb_dbg(path->tb, - "deactivating %s path from %llx:%x to %llx:%x\n", + "deactivating %s path from %llx:%u to %llx:%u\n", path->name, tb_route(path->hops[0].in_port->sw), path->hops[0].in_port->port, tb_route(path->hops[path->path_length - 1].out_port->sw), @@ -482,7 +482,7 @@ int tb_path_activate(struct tb_path *path) } tb_dbg(path->tb, - "activating %s path from %llx:%x to %llx:%x\n", + "activating %s path from %llx:%u to %llx:%u\n", path->name, tb_route(path->hops[0].in_port->sw), path->hops[0].in_port->port, tb_route(path->hops[path->path_length - 1].out_port->sw), -- 2.29.2
[PATCH 04/12] thunderbolt: Keep the parent runtime resumed for a while on device disconnect
When doing device firmware upgrade the device will disconnect for a while and then reconnect back. Keep the parent device (and the whole domain) powered for a while so we don't need to runtime resume immediately when the device is connected back after the device upgrade completes. Signed-off-by: Mika Westerberg --- drivers/thunderbolt/icm.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c index beee6e6b8b6e..635b949fb1d6 100644 --- a/drivers/thunderbolt/icm.c +++ b/drivers/thunderbolt/icm.c @@ -870,7 +870,13 @@ icm_fr_device_disconnected(struct tb *tb, const struct icm_pkg_header *hdr) return; } + pm_runtime_get_sync(sw->dev.parent); + remove_switch(sw); + + pm_runtime_mark_last_busy(sw->dev.parent); + pm_runtime_put_autosuspend(sw->dev.parent); + tb_switch_put(sw); } @@ -1280,8 +1286,13 @@ icm_tr_device_disconnected(struct tb *tb, const struct icm_pkg_header *hdr) tb_warn(tb, "no switch exists at %llx, ignoring\n", route); return; } + pm_runtime_get_sync(sw->dev.parent); remove_switch(sw); + + pm_runtime_mark_last_busy(sw->dev.parent); + pm_runtime_put_autosuspend(sw->dev.parent); + tb_switch_put(sw); } -- 2.29.2
[PATCH 02/12] thunderbolt: Log which connection manager implementation is used
This makes it easier to figure out whether the driver is using firmware or software based connection manager implementation. Signed-off-by: Mika Westerberg --- drivers/thunderbolt/icm.c | 2 ++ drivers/thunderbolt/tb.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c index 03e86817afc7..beee6e6b8b6e 100644 --- a/drivers/thunderbolt/icm.c +++ b/drivers/thunderbolt/icm.c @@ -2302,5 +2302,7 @@ struct tb *icm_probe(struct tb_nhi *nhi) return NULL; } + tb_dbg(tb, "using firmware connection manager\n"); + return tb; } diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index 214fbc92c1b7..51d5b031cada 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -1534,5 +1534,7 @@ struct tb *tb_probe(struct tb_nhi *nhi) INIT_LIST_HEAD(>dp_resources); INIT_DELAYED_WORK(>remove_work, tb_remove_work); + tb_dbg(tb, "using software connection manager\n"); + return tb; } -- 2.29.2
[PATCH 06/12] thunderbolt: Perform USB4 router NVM upgrade in two phases
The currect code expects that the router returns back the status of the
NVM authentication immediately. When tested against a real USB4 device
what happens is that the router is reset and only after that the result
is updated in the ROUTER_CS_26 register status field. This also seems to
align better what the spec suggests.
For this reason do the same what we already do with the Thunderbolt 3
devices and perform the NVM upgrade in two phases. First start the
NVM_AUTH router operation and once the router is added back after the
reset read the status in ROUTER_CS_26 and expose it to the userspace
accordingly.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/switch.c | 20 --
drivers/thunderbolt/tb.h | 1 +
drivers/thunderbolt/tb_regs.h | 1 +
drivers/thunderbolt/usb4.c| 75 +++
4 files changed, 77 insertions(+), 20 deletions(-)
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index cdfd8cccfe19..a8572f49d3ad 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2160,6 +2160,7 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)
fallthrough;
case 3:
+ case 4:
ret = tb_switch_set_uuid(sw);
if (ret)
return ret;
@@ -2175,6 +2176,22 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)
break;
}
+ if (sw->no_nvm_upgrade)
+ return 0;
+
+ if (tb_switch_is_usb4(sw)) {
+ ret = usb4_switch_nvm_authenticate_status(sw, );
+ if (ret)
+ return ret;
+
+ if (status) {
+ tb_sw_info(sw, "switch flash authentication failed\n");
+ nvm_set_auth_status(sw, status);
+ }
+
+ return 0;
+ }
+
/* Root switch DMA port requires running firmware */
if (!tb_route(sw) && !tb_switch_is_icm(sw))
return 0;
@@ -2183,9 +2200,6 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)
if (!sw->dma_port)
return 0;
- if (sw->no_nvm_upgrade)
- return 0;
-
/*
* If there is status already set then authentication failed
* when the dma_port_flash_update_auth() returned. Power cycling
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index a21000649009..3885f2515aae 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -972,6 +972,7 @@ int usb4_switch_nvm_read(struct tb_switch *sw, unsigned int
address, void *buf,
int usb4_switch_nvm_write(struct tb_switch *sw, unsigned int address,
const void *buf, size_t size);
int usb4_switch_nvm_authenticate(struct tb_switch *sw);
+int usb4_switch_nvm_authenticate_status(struct tb_switch *sw, u32 *status);
bool usb4_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in);
int usb4_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in);
int usb4_switch_dealloc_dp_resource(struct tb_switch *sw, struct tb_port *in);
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index e7d9529822fa..67cb173a2f8e 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -211,6 +211,7 @@ struct tb_regs_switch_header {
#define ROUTER_CS_90x09
#define ROUTER_CS_25 0x19
#define ROUTER_CS_26 0x1a
+#define ROUTER_CS_26_OPCODE_MASK GENMASK(15, 0)
#define ROUTER_CS_26_STATUS_MASK GENMASK(29, 24)
#define ROUTER_CS_26_STATUS_SHIFT 24
#define ROUTER_CS_26_ONS BIT(30)
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index 40f13579a3fe..d88e28eee975 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -192,7 +192,9 @@ static int usb4_switch_op(struct tb_switch *sw, u16 opcode,
u8 *status)
if (val & ROUTER_CS_26_ONS)
return -EOPNOTSUPP;
- *status = (val & ROUTER_CS_26_STATUS_MASK) >> ROUTER_CS_26_STATUS_SHIFT;
+ if (status)
+ *status = (val & ROUTER_CS_26_STATUS_MASK) >>
+ ROUTER_CS_26_STATUS_SHIFT;
return 0;
}
@@ -634,32 +636,71 @@ int usb4_switch_nvm_write(struct tb_switch *sw, unsigned
int address,
* @sw: USB4 router
*
* After the new NVM has been written via usb4_switch_nvm_write(), this
- * function triggers NVM authentication process. If the authentication
- * is successful the router is power cycled and the new NVM starts
+ * function triggers NVM authentication process. The router gets power
+ * cycled and if the authentication is successful the new NVM starts
* running. In case of failure returns negative errno.
+ *
+ * The caller should call usb4_swit
[PATCH 11/12] thunderbolt: Add USB4 router operation proxy for firmware connection manager
Intel Maple Ridge and Tiger Lake connection manager firmware implements
a USB4 router operation proxy that should be used instead of direct
register access to avoid races with the firmware. This is supported in
all firmwares where the protocol version field returned in the driver
ready response is 3 (or higher).
This adds the USB4 router proxy operations support to the driver so that
we first check the protocol version and if it is 3 (or higher) the USB4
router operation is run through the firmware provided proxy. Otherwise
the native version is used.
Most USB4 router proxy operations are pretty straightforward except
NVM_AUTH where the firmware only responds once the router is restarted
but before it sends device connected notification. To support this we
split the operation so that the reply is received asynchronously and
stored to struct icm. This last reply is then returned in
icm_usb4_switch_nvm_authenticate_status() if available.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/icm.c | 214 --
drivers/thunderbolt/tb_msgs.h | 28 +
2 files changed, 232 insertions(+), 10 deletions(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 635b949fb1d6..35935c106e3d 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -48,6 +48,18 @@ static bool start_icm;
module_param(start_icm, bool, 0444);
MODULE_PARM_DESC(start_icm, "start ICM firmware if it is not running (default:
false)");
+/**
+ * struct usb4_switch_nvm_auth - Holds USB4 NVM_AUTH status
+ * @reply: Reply from ICM firmware is placed here
+ * @request: Request that is sent to ICM firmware
+ * @icm: Pointer to ICM private data
+ */
+struct usb4_switch_nvm_auth {
+ struct icm_usb4_switch_op_response reply;
+ struct icm_usb4_switch_op request;
+ struct icm *icm;
+};
+
/**
* struct icm - Internal connection manager private data
* @request_lock: Makes sure only one message is send to ICM at time
@@ -61,6 +73,8 @@ MODULE_PARM_DESC(start_icm, "start ICM firmware if it is not
running (default: f
* @max_boot_acl: Maximum number of preboot ACL entries (%0 if not supported)
* @rpm: Does the controller support runtime PM (RTD3)
* @can_upgrade_nvm: Can the NVM firmware be upgrade on this controller
+ * @proto_version: Firmware protocol version
+ * @last_nvm_auth: Last USB4 router NVM_AUTH result (or %NULL if not set)
* @veto: Is RTD3 veto in effect
* @is_supported: Checks if we can support ICM on this controller
* @cio_reset: Trigger CIO reset
@@ -84,6 +98,8 @@ struct icm {
size_t max_boot_acl;
bool rpm;
bool can_upgrade_nvm;
+ u8 proto_version;
+ struct usb4_switch_nvm_auth *last_nvm_auth;
bool veto;
bool (*is_supported)(struct tb *tb);
int (*cio_reset)(struct tb *tb);
@@ -92,7 +108,7 @@ struct icm {
void (*save_devices)(struct tb *tb);
int (*driver_ready)(struct tb *tb,
enum tb_security_level *security_level,
- size_t *nboot_acl, bool *rpm);
+ u8 *proto_version, size_t *nboot_acl, bool *rpm);
void (*set_uuid)(struct tb *tb);
void (*device_connected)(struct tb *tb,
const struct icm_pkg_header *hdr);
@@ -437,7 +453,7 @@ static void icm_fr_save_devices(struct tb *tb)
static int
icm_fr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
- size_t *nboot_acl, bool *rpm)
+ u8 *proto_version, size_t *nboot_acl, bool *rpm)
{
struct icm_fr_pkg_driver_ready_response reply;
struct icm_pkg_driver_ready request = {
@@ -992,7 +1008,7 @@ static int icm_tr_cio_reset(struct tb *tb)
static int
icm_tr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
- size_t *nboot_acl, bool *rpm)
+ u8 *proto_version, size_t *nboot_acl, bool *rpm)
{
struct icm_tr_pkg_driver_ready_response reply;
struct icm_pkg_driver_ready request = {
@@ -1008,6 +1024,9 @@ icm_tr_driver_ready(struct tb *tb, enum tb_security_level
*security_level,
if (security_level)
*security_level = reply.info & ICM_TR_INFO_SLEVEL_MASK;
+ if (proto_version)
+ *proto_version = (reply.info & ICM_TR_INFO_PROTO_VERSION_MASK)
>>
+ ICM_TR_INFO_PROTO_VERSION_SHIFT;
if (nboot_acl)
*nboot_acl = (reply.info & ICM_TR_INFO_BOOT_ACL_MASK) >>
ICM_TR_INFO_BOOT_ACL_SHIFT;
@@ -1461,7 +1480,7 @@ static int icm_ar_get_mode(struct tb *tb)
static int
icm_ar_driver_ready(struct tb *tb, enum tb_security_level *security_level,
- size_t *nboot_acl, bool *rpm)
+ u8 *proto_version, size_t *nboot_acl, bool *rpm)
{
struct icm_ar_pkg_driver_ready_response rep
[PATCH 07/12] thunderbolt: Pass metadata directly to usb4_switch_op()
We are going to make usb4_switch_op() to match better the corresponding
firmware (ICM) USB4 router operation proxy interface, so that we can use
either based on the connection manager implementation. For this reason
pass metadata directly to usb4_switch_op().
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/usb4.c | 77 ++
1 file changed, 28 insertions(+), 49 deletions(-)
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index d88e28eee975..5f3237d66987 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -92,16 +92,6 @@ static int usb4_switch_op_write_data(struct tb_switch *sw,
const void *data,
return tb_sw_write(sw, data, TB_CFG_SWITCH, ROUTER_CS_9, dwords);
}
-static int usb4_switch_op_read_metadata(struct tb_switch *sw, u32 *metadata)
-{
- return tb_sw_read(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
-}
-
-static int usb4_switch_op_write_metadata(struct tb_switch *sw, u32 metadata)
-{
- return tb_sw_write(sw, , TB_CFG_SWITCH, ROUTER_CS_25, 1);
-}
-
static int usb4_do_read_data(u16 address, void *buf, size_t size,
read_block_fn read_block, void *read_block_data)
{
@@ -171,11 +161,18 @@ static int usb4_do_write_data(unsigned int address, const
void *buf, size_t size
return 0;
}
-static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u8 *status)
+static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u32 *metadata,
+ u8 *status)
{
u32 val;
int ret;
+ if (metadata) {
+ ret = tb_sw_write(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
+ if (ret)
+ return ret;
+ }
+
val = opcode | ROUTER_CS_26_OV;
ret = tb_sw_write(sw, , TB_CFG_SWITCH, ROUTER_CS_26, 1);
if (ret)
@@ -195,7 +192,9 @@ static int usb4_switch_op(struct tb_switch *sw, u16 opcode,
u8 *status)
if (status)
*status = (val & ROUTER_CS_26_STATUS_MASK) >>
ROUTER_CS_26_STATUS_SHIFT;
- return 0;
+
+ return metadata ?
+ tb_sw_read(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1) : 0;
}
static void usb4_switch_check_wakes(struct tb_switch *sw)
@@ -350,11 +349,7 @@ static int usb4_switch_drom_read_block(void *data,
metadata |= (dwaddress << USB4_DROM_ADDRESS_SHIFT) &
USB4_DROM_ADDRESS_MASK;
- ret = usb4_switch_op_write_metadata(sw, metadata);
- if (ret)
- return ret;
-
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_DROM_READ, );
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_DROM_READ, , );
if (ret)
return ret;
@@ -510,17 +505,14 @@ int usb4_switch_nvm_sector_size(struct tb_switch *sw)
u8 status;
int ret;
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SECTOR_SIZE, );
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SECTOR_SIZE, ,
+);
if (ret)
return ret;
if (status)
return status == 0x2 ? -EOPNOTSUPP : -EIO;
- ret = usb4_switch_op_read_metadata(sw, );
- if (ret)
- return ret;
-
return metadata & USB4_NVM_SECTOR_SIZE_MASK;
}
@@ -537,11 +529,7 @@ static int usb4_switch_nvm_read_block(void *data,
metadata |= (dwaddress << USB4_NVM_READ_OFFSET_SHIFT) &
USB4_NVM_READ_OFFSET_MASK;
- ret = usb4_switch_op_write_metadata(sw, metadata);
- if (ret)
- return ret;
-
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_READ, );
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_READ, , );
if (ret)
return ret;
@@ -579,11 +567,8 @@ static int usb4_switch_nvm_set_offset(struct tb_switch *sw,
metadata = (dwaddress << USB4_NVM_SET_OFFSET_SHIFT) &
USB4_NVM_SET_OFFSET_MASK;
- ret = usb4_switch_op_write_metadata(sw, metadata);
- if (ret)
- return ret;
-
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SET_OFFSET, );
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SET_OFFSET, ,
+);
if (ret)
return ret;
@@ -601,7 +586,7 @@ static int usb4_switch_nvm_write_next_block(void *data,
const void *buf,
if (ret)
return ret;
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_WRITE, );
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_WRITE, NULL, );
if (ret)
return ret;
@@ -648,7 +633,7 @@ int usb4_switch_nvm_authenticate(struct tb_switch *sw)
{
int ret;
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_AUTH, NULL);
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_AUTH, NULL, NULL);
switch (ret) {
/*
* The router is power cycled once NVM_AUTH i
[PATCH 05/12] thunderbolt: Return -ENOTCONN when ERR_CONN is received
This allows the calling code to distinguish if the error was due to ERR_CONN (adapter is disconneced or disabled) or something else. Will be needed in USB4 router NVM update in the following patch. Signed-off-by: Mika Westerberg --- drivers/thunderbolt/ctl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c index 1d86e27a0ef3..bac08b820015 100644 --- a/drivers/thunderbolt/ctl.c +++ b/drivers/thunderbolt/ctl.c @@ -962,6 +962,9 @@ static int tb_cfg_get_error(struct tb_ctl *ctl, enum tb_cfg_space space, if (res->tb_error == TB_CFG_ERROR_LOCK) return -EACCES; + else if (res->tb_error == TB_CFG_ERROR_PORT_NOT_CONNECTED) + return -ENOTCONN; + return -EIO; } -- 2.29.2
[PATCH 08/12] thunderbolt: Pass TX and RX data directly to usb4_switch_op()
We are going to make usb4_switch_op() to match better the corresponding
firmware (ICM) USB4 router operation proxy interface, so that we can use
either based on the connection manager implementation.
For this reason rename usb4_switch_op() to __usb4_switch_op() that
provides the most complete interface. Then make usb4_switch_op() and
usb4_switch_op_data() call it with correct set of parameters and update
the callers accordingly.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/usb4.c | 85 +-
1 file changed, 48 insertions(+), 37 deletions(-)
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index 5f3237d66987..c1bb5ec6e1db 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -74,24 +74,6 @@ static int usb4_switch_wait_for_bit(struct tb_switch *sw,
u32 offset, u32 bit,
return -ETIMEDOUT;
}
-static int usb4_switch_op_read_data(struct tb_switch *sw, void *data,
- size_t dwords)
-{
- if (dwords > USB4_DATA_DWORDS)
- return -EINVAL;
-
- return tb_sw_read(sw, data, TB_CFG_SWITCH, ROUTER_CS_9, dwords);
-}
-
-static int usb4_switch_op_write_data(struct tb_switch *sw, const void *data,
-size_t dwords)
-{
- if (dwords > USB4_DATA_DWORDS)
- return -EINVAL;
-
- return tb_sw_write(sw, data, TB_CFG_SWITCH, ROUTER_CS_9, dwords);
-}
-
static int usb4_do_read_data(u16 address, void *buf, size_t size,
read_block_fn read_block, void *read_block_data)
{
@@ -161,17 +143,27 @@ static int usb4_do_write_data(unsigned int address, const
void *buf, size_t size
return 0;
}
-static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u32 *metadata,
- u8 *status)
+static int __usb4_switch_op(struct tb_switch *sw, u16 opcode, u32 *metadata,
+ u8 *status, const void *tx_data, size_t tx_dwords,
+ void *rx_data, size_t rx_dwords)
{
u32 val;
int ret;
+ if (tx_dwords > USB4_DATA_DWORDS || rx_dwords > USB4_DATA_DWORDS)
+ return -EINVAL;
+
if (metadata) {
ret = tb_sw_write(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
if (ret)
return ret;
}
+ if (tx_dwords) {
+ ret = tb_sw_write(sw, tx_data, TB_CFG_SWITCH, ROUTER_CS_9,
+ tx_dwords);
+ if (ret)
+ return ret;
+ }
val = opcode | ROUTER_CS_26_OV;
ret = tb_sw_write(sw, , TB_CFG_SWITCH, ROUTER_CS_26, 1);
@@ -193,8 +185,34 @@ static int usb4_switch_op(struct tb_switch *sw, u16
opcode, u32 *metadata,
*status = (val & ROUTER_CS_26_STATUS_MASK) >>
ROUTER_CS_26_STATUS_SHIFT;
- return metadata ?
- tb_sw_read(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1) : 0;
+ if (metadata) {
+ ret = tb_sw_read(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
+ if (ret)
+ return ret;
+ }
+ if (rx_dwords) {
+ ret = tb_sw_read(sw, rx_data, TB_CFG_SWITCH, ROUTER_CS_9,
+rx_dwords);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static inline int usb4_switch_op(struct tb_switch *sw, u16 opcode,
+u32 *metadata, u8 *status)
+{
+ return __usb4_switch_op(sw, opcode, metadata, status, NULL, 0, NULL, 0);
+}
+
+static inline int usb4_switch_op_data(struct tb_switch *sw, u16 opcode,
+ u32 *metadata, u8 *status,
+ const void *tx_data, size_t tx_dwords,
+ void *rx_data, size_t rx_dwords)
+{
+ return __usb4_switch_op(sw, opcode, metadata, status, tx_data,
+ tx_dwords, rx_data, rx_dwords);
}
static void usb4_switch_check_wakes(struct tb_switch *sw)
@@ -349,14 +367,12 @@ static int usb4_switch_drom_read_block(void *data,
metadata |= (dwaddress << USB4_DROM_ADDRESS_SHIFT) &
USB4_DROM_ADDRESS_MASK;
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_DROM_READ, , );
+ ret = usb4_switch_op_data(sw, USB4_SWITCH_OP_DROM_READ, ,
+ , NULL, 0, buf, dwords);
if (ret)
return ret;
- if (status)
- return -EIO;
-
- return usb4_switch_op_read_data(sw, buf, dwords);
+ return status ? -EIO : 0;
}
/**
@@ -529,14 +545,12 @@ static int usb4_switch_nvm_read_block(void *data,
metadata |= (dwaddress << USB4_NVM_READ_OFFSET_SHIFT) &
USB4_NVM_READ_OFFSET_MASK;
- ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_RE
[PATCH 09/12] thunderbolt: Add connection manager specific hooks for USB4 router operations
Intel USB4 host routers that run the firmware based connection manager
(ICM) may implement a proxy for USB4 router operations. This is to avoid
the firmware to race with the OS driver, as both may need to run these
operations.
This adds two new connection manager specific callbacks which, if
provided, get called instead of the native USB4 router operation.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/tb.h | 13 ++
drivers/thunderbolt/usb4.c | 50 +-
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 3885f2515aae..d19dbc8e9457 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -367,6 +367,14 @@ struct tb_path {
* @disconnect_pcie_paths: Disconnects PCIe paths before NVM update
* @approve_xdomain_paths: Approve (establish) XDomain DMA paths
* @disconnect_xdomain_paths: Disconnect XDomain DMA paths
+ * @usb4_switch_op: Optional proxy for USB4 router operations. If set
+ * this will be called whenever USB4 router operation is
+ * performed. If this returns %-EOPNOTSUPP then the
+ * native USB4 router operation is called.
+ * @usb4_switch_nvm_authenticate_status: Optional callback that the CM
+ * implementation can be used to
+ * return status of USB4 NVM_AUTH
+ * router operation.
*/
struct tb_cm_ops {
int (*driver_ready)(struct tb *tb);
@@ -393,6 +401,11 @@ struct tb_cm_ops {
int (*disconnect_pcie_paths)(struct tb *tb);
int (*approve_xdomain_paths)(struct tb *tb, struct tb_xdomain *xd);
int (*disconnect_xdomain_paths)(struct tb *tb, struct tb_xdomain *xd);
+ int (*usb4_switch_op)(struct tb_switch *sw, u16 opcode, u32 *metadata,
+ u8 *status, const void *tx_data, size_t
tx_data_len,
+ void *rx_data, size_t rx_data_len);
+ int (*usb4_switch_nvm_authenticate_status)(struct tb_switch *sw,
+ u32 *status);
};
static inline void *tb_priv(struct tb *tb)
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index c1bb5ec6e1db..cbf1c0536360 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -143,16 +143,14 @@ static int usb4_do_write_data(unsigned int address, const
void *buf, size_t size
return 0;
}
-static int __usb4_switch_op(struct tb_switch *sw, u16 opcode, u32 *metadata,
- u8 *status, const void *tx_data, size_t tx_dwords,
- void *rx_data, size_t rx_dwords)
+static int usb4_native_switch_op(struct tb_switch *sw, u16 opcode,
+u32 *metadata, u8 *status,
+const void *tx_data, size_t tx_dwords,
+void *rx_data, size_t rx_dwords)
{
u32 val;
int ret;
- if (tx_dwords > USB4_DATA_DWORDS || rx_dwords > USB4_DATA_DWORDS)
- return -EINVAL;
-
if (metadata) {
ret = tb_sw_write(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
if (ret)
@@ -200,6 +198,39 @@ static int __usb4_switch_op(struct tb_switch *sw, u16
opcode, u32 *metadata,
return 0;
}
+static int __usb4_switch_op(struct tb_switch *sw, u16 opcode, u32 *metadata,
+ u8 *status, const void *tx_data, size_t tx_dwords,
+ void *rx_data, size_t rx_dwords)
+{
+ const struct tb_cm_ops *cm_ops = sw->tb->cm_ops;
+
+ if (tx_dwords > USB4_DATA_DWORDS || rx_dwords > USB4_DATA_DWORDS)
+ return -EINVAL;
+
+ /*
+* If the connection manager implementation provides USB4 router
+* operation proxy callback, call it here instead of running the
+* operation natively.
+*/
+ if (cm_ops->usb4_switch_op) {
+ int ret;
+
+ ret = cm_ops->usb4_switch_op(sw, opcode, metadata, status,
+tx_data, tx_dwords, rx_data,
+rx_dwords);
+ if (ret != -EOPNOTSUPP)
+ return ret;
+
+ /*
+* If the proxy was not supported then run the native
+* router operation instead.
+*/
+ }
+
+ return usb4_native_switch_op(sw, opcode, metadata, status, tx_data,
+tx_dwords, rx_data, rx_dwords);
+}
+
static inline int usb4_switch_op(struct tb_switch *sw, u16 opcode,
u32 *metadata, u8 *status)
{
@@ -674,10 +705,17 @@ int usb4_switch_nvm_authenticate(struct tb_switch *sw)
*/
int usb4_switch_nvm_authenticate_status(struct tb_switch *sw, u32 *status)
[PATCH 10/12] thunderbolt: Move constants for USB4 router operations to tb_regs.h
We are going to use these in subsequent patch so make them available
outside of usb4.c.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/tb_regs.h | 13 +
drivers/thunderbolt/usb4.c| 12
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 67cb173a2f8e..ae427a953489 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -217,6 +217,19 @@ struct tb_regs_switch_header {
#define ROUTER_CS_26_ONS BIT(30)
#define ROUTER_CS_26_OVBIT(31)
+/* USB4 router operations opcodes */
+enum usb4_switch_op {
+ USB4_SWITCH_OP_QUERY_DP_RESOURCE = 0x10,
+ USB4_SWITCH_OP_ALLOC_DP_RESOURCE = 0x11,
+ USB4_SWITCH_OP_DEALLOC_DP_RESOURCE = 0x12,
+ USB4_SWITCH_OP_NVM_WRITE = 0x20,
+ USB4_SWITCH_OP_NVM_AUTH = 0x21,
+ USB4_SWITCH_OP_NVM_READ = 0x22,
+ USB4_SWITCH_OP_NVM_SET_OFFSET = 0x23,
+ USB4_SWITCH_OP_DROM_READ = 0x24,
+ USB4_SWITCH_OP_NVM_SECTOR_SIZE = 0x25,
+};
+
/* Router TMU configuration */
#define TMU_RTR_CS_0 0x00
#define TMU_RTR_CS_0_TDBIT(27)
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index cbf1c0536360..6a0aa83a1ac8 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -16,18 +16,6 @@
#define USB4_DATA_DWORDS 16
#define USB4_DATA_RETRIES 3
-enum usb4_switch_op {
- USB4_SWITCH_OP_QUERY_DP_RESOURCE = 0x10,
- USB4_SWITCH_OP_ALLOC_DP_RESOURCE = 0x11,
- USB4_SWITCH_OP_DEALLOC_DP_RESOURCE = 0x12,
- USB4_SWITCH_OP_NVM_WRITE = 0x20,
- USB4_SWITCH_OP_NVM_AUTH = 0x21,
- USB4_SWITCH_OP_NVM_READ = 0x22,
- USB4_SWITCH_OP_NVM_SET_OFFSET = 0x23,
- USB4_SWITCH_OP_DROM_READ = 0x24,
- USB4_SWITCH_OP_NVM_SECTOR_SIZE = 0x25,
-};
-
enum usb4_sb_target {
USB4_SB_TARGET_ROUTER,
USB4_SB_TARGET_PARTNER,
--
2.29.2
[PATCH 12/12] thunderbolt: Add support for Intel Maple Ridge
Maple Ridge is first discrete USB4 host controller from Intel. It comes
with firmware based connection manager and the flows are similar as used
in Intel Titan Ridge.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/icm.c | 11 +++
drivers/thunderbolt/nhi.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 35935c106e3d..adc7b61937a1 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -2499,6 +2499,17 @@ struct tb *icm_probe(struct tb_nhi *nhi)
icm->rtd3_veto = icm_icl_rtd3_veto;
tb->cm_ops = _icl_ops;
break;
+
+ case PCI_DEVICE_ID_INTEL_MAPLE_RIDGE_4C_NHI:
+ icm->is_supported = icm_tgl_is_supported;
+ icm->get_mode = icm_ar_get_mode;
+ icm->driver_ready = icm_tr_driver_ready;
+ icm->device_connected = icm_tr_device_connected;
+ icm->device_disconnected = icm_tr_device_disconnected;
+ icm->xdomain_connected = icm_tr_xdomain_connected;
+ icm->xdomain_disconnected = icm_tr_xdomain_disconnected;
+ tb->cm_ops = _tr_ops;
+ break;
}
if (!icm->is_supported || !icm->is_supported(tb)) {
diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 80162e4b013f..ffe0531c0fd0 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -55,6 +55,7 @@ extern const struct tb_nhi_ops icl_nhi_ops;
* need for the PCI quirk anymore as we will use ICM also on Apple
* hardware.
*/
+#define PCI_DEVICE_ID_INTEL_MAPLE_RIDGE_4C_NHI 0x1137
#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI0x157d
#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE 0x157e
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI0x15bf
--
2.29.2
[PATCH 01/12] thunderbolt: Move max_boot_acl field to correct place in struct icm
This makes the kernel-doc to match the ordering and also this is better
place for it, not between upstream_port and vnd_cap that are used
together.
Signed-off-by: Mika Westerberg
---
drivers/thunderbolt/icm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index b51fc3f62b1f..03e86817afc7 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -79,9 +79,9 @@ struct icm {
struct mutex request_lock;
struct delayed_work rescan_work;
struct pci_dev *upstream_port;
- size_t max_boot_acl;
int vnd_cap;
bool safe_mode;
+ size_t max_boot_acl;
bool rpm;
bool can_upgrade_nvm;
bool veto;
--
2.29.2
[PATCH 00/12] thunderbolt: USB4 NVM upgrade improvements & Maple Ridge support
Hi all, This series improves the USB4 router NVM upgrade functionality and adds support for USB4 router operations proxy implemented by recent Intel Thunderbolt firmware connection manager. The last patch adds support for Intel Maple Ridge that is the first discrete Thunderbolt/USB4 controller from Intel. This also includes a couple of minor cleanups and improvements around debug logging. Mika Westerberg (12): thunderbolt: Move max_boot_acl field to correct place in struct icm thunderbolt: Log which connection manager implementation is used thunderbolt: Log adapter numbers in decimal in path activation/deactivation thunderbolt: Keep the parent runtime resumed for a while on device disconnect thunderbolt: Return -ENOTCONN when ERR_CONN is received thunderbolt: Perform USB4 router NVM upgrade in two phases thunderbolt: Pass metadata directly to usb4_switch_op() thunderbolt: Pass TX and RX data directly to usb4_switch_op() thunderbolt: Add connection manager specific hooks for USB4 router operations thunderbolt: Move constants for USB4 router operations to tb_regs.h thunderbolt: Add USB4 router operation proxy for firmware connection manager thunderbolt: Add support for Intel Maple Ridge drivers/thunderbolt/ctl.c | 3 + drivers/thunderbolt/icm.c | 240 -- drivers/thunderbolt/nhi.h | 1 + drivers/thunderbolt/path.c| 4 +- drivers/thunderbolt/switch.c | 20 ++- drivers/thunderbolt/tb.c | 2 + drivers/thunderbolt/tb.h | 14 ++ drivers/thunderbolt/tb_msgs.h | 28 drivers/thunderbolt/tb_regs.h | 14 ++ drivers/thunderbolt/usb4.c| 269 -- 10 files changed, 473 insertions(+), 122 deletions(-) -- 2.29.2
Re: [PATCH v3] Documentation: ACPI: explain how to use gpio-line-names
On Thu, Nov 12, 2020 at 01:52:34PM +0100, Flavio Suligoi wrote: > The "gpio-line-names" declaration is not fully > documented, so can be useful to add some important > information and one more example. > > This commit also fixes a trivial spelling mistake. > > Signed-off-by: Flavio Suligoi > Reviewed-by: Andy Shevchenko Reviewed-by: Mika Westerberg
Re: [PATCH v2] pinctrl: intel: Fix Jasperlake HOSTSW_OWN offset
On Wed, Nov 11, 2020 at 03:17:28PM -0800, Evan Green wrote:
> GPIOs that attempt to use interrupts get thwarted with a message like:
> "pin 161 cannot be used as IRQ" (for instance with SD_CD). This is because
> the HOSTSW_OWN offset is incorrect, so every GPIO looks like it's
> owned by ACPI.
>
> Fixes: e278dcb7048b1 ("pinctrl: intel: Add Intel Jasper Lake pin controller
> support")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Evan Green
Acked-by: Mika Westerberg
Re: How to enable auto-suspend by default
On Wed, Nov 11, 2020 at 12:27:32PM +0100, Hans de Goede wrote: > Hi, > > On 11/10/20 6:25 PM, Mika Westerberg wrote: > > On Tue, Nov 10, 2020 at 04:02:33PM +, Limonciello, Mario wrote: > >>> > >>> On Tue, Nov 10, 2020 at 11:57:07AM +0100, Bastien Nocera wrote: > >>>> Hey, > >>>> > >>>> systemd has been shipping this script to enable auto-suspend on a > >>>> number of USB and PCI devices: > >>>> > >>> https://github.com/systemd/systemd/blob/master/tools/chromiumos/gen_autosuspen > >>> d_rules.py > >>>> > >>>> The problem here is twofold. First, the list of devices is updated from > >>>> ChromeOS, and the original list obviously won't be updated by ChromeOS > >>>> developers unless a device listed exists in a ChromeBook computer, > >>>> which means a number of devices that do support autosuspend aren't > >>>> listed. > >>>> > >>>> The other problem is that this list needs to exist at all, and that it > >>>> doesn't seem possible for device driver developers (at various levels > >>>> of the stack) to opt-in to auto-suspend when all the variants of the > >>>> device (or at least detectable ones) support auto-suspend. > >>> > >>> A driver can say they support autosuspend today, but I think you are > >>> concerned about the devices that are controlled by class-compliant > >>> drivers, right? And for those, no, we can't do this in the kernel as > >>> there are just too many broken devices out there. > >>> > >> > >> I guess what Bastien is getting at is for newer devices supported by class > >> drivers rather than having to store an allowlist in udev rules, can we set > >> the allowlist in the kernel instead. Then distributions that either don't > >> use systemd or don't regularly update udev rules from systemd can take > >> advantage of better defaults on modern hardware. > >> > >> The one item that stood out to me in that rules file was 8086:a0ed. > >> It's listed as "Volteer XHCI", but that same device ID is actually present > >> in an XPS 9310 in front of me as well and used by the xhci-pci kernel > >> module. > >> > >> Given we're effectively ending up with the combination of runtime PM turned > >> on by udev rules, do we need something like this for that ID: > >> > >> https://github.com/torvalds/linux/commit/6a7c533d4a1854f54901a065d8c672e890400d8a > >> > >> @Mika Westerberg should 8086:a0ed be quirked like the TCSS xHCI too? > > > > I think this one is the TGL PCH xHCI. The quirk currently for xHCI > > controllers that are part of the TCSS (Type-C SubSystem) where it is > > important to put all devices into low power mode whenever possible, > > otherwise it keeps the whole block on. > > Note that there are currently some IDs missing from the xHCIs which > are part of the TCSS too. At least the id for the xHCI in the thunderbolt > controller on the Lenovo T14 gen 1 is missing. I started a discussion > about extending the kernel quirk list for this vs switching to hwdb > a while a go: > > https://lore.kernel.org/linux-usb/b8b21ba3-0a8a-ff54-5e12-cf8960651...@redhat.com/ > > The conclusion back then was to switch to hwdb, but I never got around to > this. The reason I've added these to the xHCI driver is that it works even if you are running some really small userspace (like busybox). Also for the xHCI in TCSS we know for sure that it fully supports D3cold. (The one you refer above is actually mistake from my side as I never tested Alpine Ridge LP controller which I think this is). > > Typically we haven't done that for PCH side xHCI controllers though, but > > I don't see why not if it works that is. Adding Mathias to comment more > > on that since he is the xHCI maintainer. > > If we are also going to enable this for the non TCSS Intel XHCI controllers, > maybe just uncondtionally enable it for all Intel XHCI controllers, or > if necessary do a deny-list for some older models and enable it for anything > not on the deny-list (so all newer models). That should avoid the game of > whack-a-mole which we will have with this otherwise. This is really up to Mathias to decide. I'm fine either way :)
Re: How to enable auto-suspend by default
On Tue, Nov 10, 2020 at 04:02:33PM +, Limonciello, Mario wrote: > > > > On Tue, Nov 10, 2020 at 11:57:07AM +0100, Bastien Nocera wrote: > > > Hey, > > > > > > systemd has been shipping this script to enable auto-suspend on a > > > number of USB and PCI devices: > > > > > https://github.com/systemd/systemd/blob/master/tools/chromiumos/gen_autosuspen > > d_rules.py > > > > > > The problem here is twofold. First, the list of devices is updated from > > > ChromeOS, and the original list obviously won't be updated by ChromeOS > > > developers unless a device listed exists in a ChromeBook computer, > > > which means a number of devices that do support autosuspend aren't > > > listed. > > > > > > The other problem is that this list needs to exist at all, and that it > > > doesn't seem possible for device driver developers (at various levels > > > of the stack) to opt-in to auto-suspend when all the variants of the > > > device (or at least detectable ones) support auto-suspend. > > > > A driver can say they support autosuspend today, but I think you are > > concerned about the devices that are controlled by class-compliant > > drivers, right? And for those, no, we can't do this in the kernel as > > there are just too many broken devices out there. > > > > I guess what Bastien is getting at is for newer devices supported by class > drivers rather than having to store an allowlist in udev rules, can we set > the allowlist in the kernel instead. Then distributions that either don't > use systemd or don't regularly update udev rules from systemd can take > advantage of better defaults on modern hardware. > > The one item that stood out to me in that rules file was 8086:a0ed. > It's listed as "Volteer XHCI", but that same device ID is actually present > in an XPS 9310 in front of me as well and used by the xhci-pci kernel module. > > Given we're effectively ending up with the combination of runtime PM turned > on by udev rules, do we need something like this for that ID: > > https://github.com/torvalds/linux/commit/6a7c533d4a1854f54901a065d8c672e890400d8a > > @Mika Westerberg should 8086:a0ed be quirked like the TCSS xHCI too? I think this one is the TGL PCH xHCI. The quirk currently for xHCI controllers that are part of the TCSS (Type-C SubSystem) where it is important to put all devices into low power mode whenever possible, otherwise it keeps the whole block on. Typically we haven't done that for PCH side xHCI controllers though, but I don't see why not if it works that is. Adding Mathias to comment more on that since he is the xHCI maintainer.
Re: [PATCH v1] pinctrl: intel: Add Intel Alder Lake pin controller support
Hi, I think the $subject should say "Alder Lake-S" as this is for -S variant. On Mon, Oct 26, 2020 at 09:25:52PM +0200, Andy Shevchenko wrote: > This driver adds pinctrl/GPIO support for Intel Alder Lake SoC. The > GPIO controller is based on the next generation GPIO hardware but still > compatible with the one supported by the Intel core pinctrl/GPIO driver. > > Signed-off-by: Andy Shevchenko > --- > drivers/pinctrl/intel/Kconfig | 9 + > drivers/pinctrl/intel/Makefile| 1 + > drivers/pinctrl/intel/pinctrl-alderlake.c | 437 ++ > 3 files changed, 447 insertions(+) > create mode 100644 drivers/pinctrl/intel/pinctrl-alderlake.c > > diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig > index b9d78a4187e0..98494c8fdaf8 100644 > --- a/drivers/pinctrl/intel/Kconfig > +++ b/drivers/pinctrl/intel/Kconfig > @@ -70,6 +70,14 @@ config PINCTRL_INTEL > select GPIOLIB > select GPIOLIB_IRQCHIP > > +config PINCTRL_ALDERLAKE > + tristate "Intel Alder Lake pinctrl and GPIO driver" > + depends on ACPI > + select PINCTRL_INTEL > + help > + This pinctrl driver provides an interface that allows configuring > + of Intel Alder Lake PCH pins and using them as GPIOs. > + > config PINCTRL_BROXTON > tristate "Intel Broxton pinctrl and GPIO driver" > depends on ACPI > @@ -158,4 +166,5 @@ config PINCTRL_TIGERLAKE > help > This pinctrl driver provides an interface that allows configuring > of Intel Tiger Lake PCH pins and using them as GPIOs. > + Is this intentional ws change? > endif Otherwise looks good to me.
Re: [PATCH v3] thunderbolt: Add the missed ida_simple_remove() in ring_request_msix()
On Thu, Oct 15, 2020 at 04:40:53PM +0800, Jing Xiangfeng wrote:
> ring_request_msix() misses to call ida_simple_remove() in an error path.
> Add a label 'err_ida_remove' and jump to it.
>
> Fixes: 046bee1f9ab8 ("thunderbolt: Add MSI-X support")
> Signed-off-by: Jing Xiangfeng
Applied to thunderbolt.git/fixes, thanks!
Re: [PATCH] thunderbolt: use kobj_to_dev()
On Tue, Sep 22, 2020 at 08:18:13PM +0800, Wang Qing wrote: > Use kobj_to_dev() instead of container_of() > > Signed-off-by: Wang Qing Thanks but I have already queued a similar patch from Tian Tao: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git/commit/?h=next=fff15f23b8e74c2f969a5d25f29d0565e76e7137
Re: [PATCH next v2] gpiolib: check for parent device in devprop_gpiochip_set_names()
On Thu, Sep 17, 2020 at 09:48:57AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski
>
> It's possible for a GPIO chip to not have a parent device (whose
> properties we inspect for 'gpio-line-names'). In this case we should
> simply return from devprop_gpiochip_set_names(). Add an appropriate
> check for this use-case.
>
> Fixes: 7cba1a4d5e16 ("gpiolib: generalize devprop_gpiochip_set_names() for
> device properties")
> Reported-by: Anders Roxell
> Signed-off-by: Bartosz Golaszewski
> Reviewed-by: Andy Shevchenko
> Tested-by: Anders Roxell
FWIW,
Reviewed-by: Mika Westerberg
Re: [PATCH v2 0/3] gpiolib: generalize GPIO line names property
On Wed, Sep 09, 2020 at 10:54:23AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > I initially sent this as part of the gpio-mockup overhaul but since > these patches are indepentent and the work on gpio-mockup may become > more complicated - I'm sending these separately. > > The only change is adding additional property helpers to count strings > in array. > > v1 -> v2: > - actually remove the previous devprop source file in patch 3 > - rename the string counting functions to something more explicit > > Bartosz Golaszewski (3): > device: property: add helpers to count items in string arrays > gpiolib: generalize devprop_gpiochip_set_names() for device properties > gpiolib: unexport devprop_gpiochip_set_names() The series looks good to me, Reviewed-by: Mika Westerberg
Re: [PATCH v2] PM: sleep: core: Fix the handling of pending runtime resume requests
Hi Rafael,
On Mon, Aug 24, 2020 at 07:35:31PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> It has been reported that system-wide suspend may be aborted in the
> absence of any wakeup events due to unforseen interactions of it with
> the runtume PM framework.
>
> One failing scenario is when there are multiple devices sharing an
> ACPI power resource and runtime-resume needs to be carried out for
> one of them during system-wide suspend (for example, because it needs
> to be reconfigured before the whole system goes to sleep). In that
> case, the runtime-resume of that device involves turning the ACPI
> power resource "on" which in turn causes runtime-resume requests
> to be queued up for all of the other devices sharing it. Those
> requests go to the runtime PM workqueue which is frozen during
> system-wide suspend, so they are not actually taken care of until
> the resume of the whole system, but the pm_runtime_barrier()
> call in __device_suspend() sees them and triggers system wakeup
> events for them which then cause the system-wide suspend to be
> aborted if wakeup source objects are in active use.
>
> Of course, the logic that leads to triggering those wakeup events is
> questionable in the first place, because clearly there are cases in
> which a pending runtime resume request for a device is not connected
> to any real wakeup events in any way (like the one above). Moreover,
> it is racy, because the device may be resuming already by the time
> the pm_runtime_barrier() runs and so if the driver doesn't take care
> of signaling the wakeup event as appropriate, it will be lost.
> However, if the driver does take care of that, the extra
> pm_wakeup_event() call in the core is redundant.
>
> Accordingly, drop the conditional pm_wakeup_event() call fron
> __device_suspend() and make the latter call pm_runtime_barrier()
> alone. Also modify the comment next to that call to reflect the new
> code and extend it to mention the need to avoid unwanted interactions
> between runtime PM and system-wide device suspend callbacks.
>
> Fixes: 1e2ef05bb8cf8 ("PM: Limit race conditions between runtime PM and
> system sleep (v2)")
> Reported-by: Mika Westerberg
I guess the more correct here is
Reported-by: Utkarsh H Patel
> Signed-off-by: Rafael J. Wysocki
I also got confirmation that this fixes the reported issue and did not
seem to cause regressions either :)
Please add the following tags:
Tested-by: Utkarsh H Patel
Tested-by: Pengfei Xu
Thanks for fixing this!
Re: [PATCH] ACPI: OSL: Prevent acpi_release_memory() from returning too early
On Fri, Aug 21, 2020 at 07:42:55PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> After commit 1757659d022b ("ACPI: OSL: Implement deferred unmapping
> of ACPI memory") in some cases acpi_release_memory() may return
> before the target memory mappings actually go away, because they
> are released asynchronously now.
>
> Prevent it from returning prematurely by making it wait for the next
> RCU grace period to elapse, for all of the RCU callbacks to complete
> and for all of the scheduled work items to be flushed before
> returning.
>
> Fixes: 1757659d022b ("ACPI: OSL: Implement deferred unmapping of ACPI memory")
> Reported-by: Kenneth R. Crudup
> Signed-off-by: Rafael J. Wysocki
Reviewed-by: Mika Westerberg
Re: [PATCH] PM: sleep: core: Fix the handling of pending runtime resume requests
On Mon, Aug 24, 2020 at 03:38:39PM +0200, Rafael J. Wysocki wrote: > BTW, does the patch make the issue at hand go away? I asked the folks who have this particular hardware to try it out as I don't have access to this one. Hopefully we get the results back soon and once we do, I'll let you know.
Re: [PATCH] PCI/ASPM: Enable ASPM for links under VMD domain
Hi,
On Fri, Aug 21, 2020 at 08:32:20PM +0800, Kai-Heng Feng wrote:
> New Intel laptops with VMD cannot reach deeper power saving state,
> renders very short battery time.
>
> As BIOS may not be able to program the config space for devices under
> VMD domain, ASPM needs to be programmed manually by software. This is
> also the case under Windows.
>
> The VMD controller itself is a root complex integrated endpoint that
> doesn't have ASPM capability, so we can't propagate the ASPM settings to
> devices under it. Hence, simply apply ASPM_STATE_ALL to the links under
> VMD domain, unsupported states will be cleared out anyway.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/pci/pcie/aspm.c | 3 ++-
> drivers/pci/quirks.c| 11 +++
> include/linux/pci.h | 2 ++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 253c30cc1967..dcc002dbca19 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -624,7 +624,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> *link, int blacklist)
> aspm_calc_l1ss_info(link, , );
>
> /* Save default state */
> - link->aspm_default = link->aspm_enabled;
> + link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> + ASPM_STATE_ALL : link->aspm_enabled;
>
> /* Setup initial capable state. Will be updated later */
> link->aspm_capable = link->aspm_support;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index bdf9b52567e0..2e2f525bd892 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5632,3 +5632,14 @@ static void apex_pci_fixup_class(struct pci_dev *pdev)
> }
> DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
> PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
> +
> +/*
> + * Device [8086:9a09]
> + * BIOS may not be able to access config space of devices under VMD domain,
> so
> + * it relies on software to enable ASPM for links under VMD.
> + */
> +static void pci_fixup_enable_aspm(struct pci_dev *pdev)
> +{
> + pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 835530605c0d..66a45916c7c6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -227,6 +227,8 @@ enum pci_dev_flags {
> PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> /* Don't use Relaxed Ordering for TLPs directed at this device */
> PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> + /* Enable ASPM regardless of how LnkCtl is programmed */
> + PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 12),
I wonder if instead of dev_flags this should have a bit field in struct
pci_dev? Not sure which one is prefered actually, both seem to include
quirks as well ;-)
> };
>
> enum pci_irq_reroute_variant {
> --
> 2.17.1
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Mon, Aug 24, 2020 at 11:31:40AM +0200, Arnd Bergmann wrote:
> On Mon, Aug 24, 2020 at 11:15 AM Mika Westerberg
> wrote:
> > On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> > > On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> > > wrote:
> > > > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > > > >
> > > > > The mtd core just checks both the permissions on the device node
> > > > > (which
> > > > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > > > on the underlying device that is controlled by the module parameter
> > > > > in case of intel-spi{,-platform,-pci}.c.
> > > >
> > > > OK, thanks.
> > > >
> > > > Since we cannot really get rid of the module parameter (AFAIK there are
> > > > users for it), I still think we should just make the "writeable" to
> > > > apply to the PCI part as well. That should at least make it consistent,
> > > > and it also solves Daniel's case.
> > >
> > > Can you explain Daniel's case then? I still don't understand what he
> > > actually wants.
> > >
> > > As I keep repeating, the module parameter *does* apply to the pci
> > > driver front-end since it determines whether the driver will disallow
> > > writes to the mtd device without it. The only difference is that the pci
> > > driver will attempt to set the hardware bit without checking the
> > > module parameter first, while the platform driver does not. If the
> > > module parameter is not set however, the state of the hardware
> > > bit is never checked again.
> >
> > I think Daniel wants the PCI driver not to set the hardware bit by
> > default (same as the platform driver).
>
> Sure, but *why*?
Because this is part of the platform firmware security check patch he is
also working on and, I guess making the flash chip writeable by default
is triggering some of the checks in that patch. Or something along those
lines ;-)
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Mon, Aug 24, 2020 at 11:08:33AM +0200, Arnd Bergmann wrote:
> On Mon, Aug 24, 2020 at 10:22 AM Mika Westerberg
> wrote:
> > On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> > > On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> > >
> > > The mtd core just checks both the permissions on the device node (which
> > > default to 0600 without any special udev rules) and the MTD_WRITEABLE
> > > on the underlying device that is controlled by the module parameter
> > > in case of intel-spi{,-platform,-pci}.c.
> >
> > OK, thanks.
> >
> > Since we cannot really get rid of the module parameter (AFAIK there are
> > users for it), I still think we should just make the "writeable" to
> > apply to the PCI part as well. That should at least make it consistent,
> > and it also solves Daniel's case.
>
> Can you explain Daniel's case then? I still don't understand what he
> actually wants.
>
> As I keep repeating, the module parameter *does* apply to the pci
> driver front-end since it determines whether the driver will disallow
> writes to the mtd device without it. The only difference is that the pci
> driver will attempt to set the hardware bit without checking the
> module parameter first, while the platform driver does not. If the
> module parameter is not set however, the state of the hardware
> bit is never checked again.
I think Daniel wants the PCI driver not to set the hardware bit by
default (same as the platform driver).
Re: [PATCH] PM: sleep: core: Fix the handling of pending runtime resume requests
Hi, On Fri, Aug 21, 2020 at 03:34:42PM -0400, Alan Stern wrote: > This means that the code could be simplified to just: > > pm_runtime_barrier(dev); > > Will this fix the reported bug? It seems likely to me that the actual > problem with the failure scenario in the patch description was that > turning on an ACPI power resource causes runtime-resume requests to be > queued for all devices sharing that resource. Wouldn't it make more > sense to resume only the device that requested it and leave the others > in runtime suspend? The problem with at least PCIe devices that share ACPI power resources is that once you turn on the power resource all the devices that shared it will go into D0uninitialized power state and that means they lose all wake configuration etc. so they need to be re-initialized by their driver before they can go back to D3(cold) in order for their wakes to still work.
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Sat, Aug 22, 2020 at 06:06:03PM +0200, Arnd Bergmann wrote:
> On Wed, Aug 19, 2020 at 11:11 AM Mika Westerberg
> wrote:
> > On Wed, Aug 19, 2020 at 10:38:24AM +0200, Arnd Bergmann wrote:
> > > On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg
> > > wrote:
> >
> > > > Actually thinking about this bit more, to make PCI and the platform
> > > > parts consistent we can make the "writeable" control this for the PCI
> > > > part as well. So what if we add a callback to struct intel_spi_boardinfo
> > > > that the PCI driver populates and then the "core" driver uses to enable
> > > > writing when "writeable" is set to 1.
> > >
> > > If you are really worried about the write protection being bypassed by
> > > a different driver or code injection, the best way would seem to be to
> > > only enable writing in the mtd write callback and disable it immediately
> > > after the write is complete. I still don't see why this hardware would
> > > be more susceptible to this kind of attack than other drivers though,
> > > as it already has the safeguard against writing through the MTD layer
> > > without the module parameter.
> >
> > Hmm, is there already a mechanism at the MTD level to prevent writes? If
> > that's the case then sure we can use that instead.
>
> The mtd core just checks both the permissions on the device node (which
> default to 0600 without any special udev rules) and the MTD_WRITEABLE
> on the underlying device that is controlled by the module parameter
> in case of intel-spi{,-platform,-pci}.c.
OK, thanks.
Since we cannot really get rid of the module parameter (AFAIK there are
users for it), I still think we should just make the "writeable" to
apply to the PCI part as well. That should at least make it consistent,
and it also solves Daniel's case.
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Wed, Aug 19, 2020 at 10:38:24AM +0200, Arnd Bergmann wrote: > On Wed, Aug 19, 2020 at 8:57 AM Mika Westerberg > wrote: > > > > On Tue, Aug 18, 2020 at 12:55:59PM -0300, Daniel Gutson wrote: > > > > If you care about other (malicious) code writing to the driver, please > > > > explain > > > > what the specific attack scenario is that you are worried about, and > > > > why you think > > > > this is not sufficient. What code would be able to write to the device > > > > if not the > > > > device driver itself? > > > > > > Maybe Mika can answer this better, but what I'm trying to do is to > > > limit the possibility of > > > damage, as explained in the Kconfig: > > > "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)" > > > "Say N here unless you know what you are doing. Overwriting the > > > SPI flash may render the system unbootable." > > > > Right, the PCI part of the driver unconditionally (and wrongly) tried to > > set the chip writeable. > > > > What this whole thing tries to protect is that the user does not > > accidentally write to the flash chip. It contains BIOS and other > > important firmware so touching it (if it is not locked in the BIOS side) > > may potentially brick the system. That's why we also require that > > command line parameter so the user who knows what he or she is doing can > > enable it for writing. > > The same thing can happen with the platform driver if you load it > once with 'writeable=1' and then unload, leaving the chip in writeable > state. If you load it a second time without the module parameter, it > will be in the same state as the PCI driver: the hardware bit allows > writing, but the MTD layer prevents writes from being issued to the > device. Right. > > Actually thinking about this bit more, to make PCI and the platform > > parts consistent we can make the "writeable" control this for the PCI > > part as well. So what if we add a callback to struct intel_spi_boardinfo > > that the PCI driver populates and then the "core" driver uses to enable > > writing when "writeable" is set to 1. > > If you are really worried about the write protection being bypassed by > a different driver or code injection, the best way would seem to be to > only enable writing in the mtd write callback and disable it immediately > after the write is complete. I still don't see why this hardware would > be more susceptible to this kind of attack than other drivers though, > as it already has the safeguard against writing through the MTD layer > without the module parameter. Hmm, is there already a mechanism at the MTD level to prevent writes? If that's the case then sure we can use that instead.
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Tue, Aug 18, 2020 at 12:55:59PM -0300, Daniel Gutson wrote: > > If you care about other (malicious) code writing to the driver, please > > explain > > what the specific attack scenario is that you are worried about, and > > why you think > > this is not sufficient. What code would be able to write to the device > > if not the > > device driver itself? > > Maybe Mika can answer this better, but what I'm trying to do is to > limit the possibility of > damage, as explained in the Kconfig: > "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)" > "Say N here unless you know what you are doing. Overwriting the > SPI flash may render the system unbootable." Right, the PCI part of the driver unconditionally (and wrongly) tried to set the chip writeable. What this whole thing tries to protect is that the user does not accidentally write to the flash chip. It contains BIOS and other important firmware so touching it (if it is not locked in the BIOS side) may potentially brick the system. That's why we also require that command line parameter so the user who knows what he or she is doing can enable it for writing. Actually thinking about this bit more, to make PCI and the platform parts consistent we can make the "writeable" control this for the PCI part as well. So what if we add a callback to struct intel_spi_boardinfo that the PCI driver populates and then the "core" driver uses to enable writing when "writeable" is set to 1.
Re: linux-next: build failure after merge of the thunderbolt tree
Hi all, On Mon, Aug 10, 2020 at 09:10:53AM +1000, Stephen Rothwell wrote: > Hi Linus, > > On Sun, 9 Aug 2020 11:04:28 -0700 Linus Torvalds > wrote: > > > > On Sun, Aug 9, 2020 at 1:19 AM Stephen Rothwell > > wrote: > > > > > > I looks like the above report got lost along the way to you :-( > > > > Hmm. Why didn't I see this as a build failure? > > > > Oh. Because the USB4_KUNIT_TEST stuff requires everything to be built > > in. And I have them as modules. > > Yeah, I only saw it because I do an allyesconfig build late in my day. Sorry about this. I was not sure how to deal with this as Thunderbolt goes through Greg's USB tree, so I just ended up mentioning it in my pull request without actually including the complete patch. The reason for the built in dependency is that KUnit adds its own module_init() in kunit_test_suite() so that causes linker error since the TBT driver does the same. I guess the better alternative would be to not use kunit_test_suite() and instead make calls to __kunit_test_suites_init() and __kunit_test_suites_exit() in the driver's nhi_init()/nhi_exit(). > > > Here's the patch in case you want to directly apply it: > > > > Will do. Thanks, > > Thanks. Thanks!
Re: [PATCH] mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
On Tue, Aug 04, 2020 at 10:58:17AM -0300, Daniel Gutson wrote: > Currently, the intel-spi-pci driver tries to unconditionally set > the SPI chip writeable. After discussing in the LKML, the original > author decided that it was better to remove the attempt. > > Context, the intel-spi has a module argument that controls > whether the driver attempts to turn the SPI flash chip writeable. > The default value is FALSE (don't try to make it writeable). > However, this flag applies only for a number of devices, coming from the > platform driver, whereas the devices detected through the PCI driver > (intel-spi-pci) are not subject to this check since the configuration > takes place in intel-spi-pci which doesn't have an argument. > > This patch removes the code that attempts to turn the SPI chip writeable. > > Signed-off-by: Daniel Gutson Reviewed-by: Mika Westerberg
Re: [PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable
Hi,
On Mon, Aug 03, 2020 at 10:44:49AM -0300, Daniel Gutson wrote:
> Currently, the intel-spi-pci driver tries to unconditionally set
> the SPI chip writeable. After discussing in the LKML, the original
> author decided that it was better to remove the attempt.
>
> Context, the intel-spi has a module argument that controls
> whether the driver attempts to turn the SPI flash chip writeable.
> The default value is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.
>
> This patch removes the code that attempts to turn the SPI chip writeable.
I think you should make the $subject to follow the convention used in
the SPI-NOR subsystem. You can see it running following command:
$ git log --oneline drivers/mtd/spi-nor/controllers/intel-spi-pci.c
In this case I think it should be:
mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable
or something like that.
The patch itself looks good, one minor comment below.
>
> Signed-off-by: Daniel Gutson
> ---
> drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 81329f680bec..d721ba4e8b41 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -41,13 +41,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> if (!info)
> return -ENOMEM;
>
> - /* Try to make the chip read/write */
> pci_read_config_dword(pdev, BCR, );
> - if (!(bcr & BCR_WPD)) {
> - bcr |= BCR_WPD;
> - pci_write_config_dword(pdev, BCR, bcr);
> - pci_read_config_dword(pdev, BCR, );
> - }
> info->writeable = !!(bcr & BCR_WPD);
Perhaps we should log this in debug level (dev_dbg()) so when debugging
possible issues we can see that the chip is write protected by the BIOS.
>
> ispi = intel_spi_probe(>dev, >resource[0], info);
> --
> 2.25.1
Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable
On Mon, Aug 03, 2020 at 09:58:23AM -0300, Daniel Gutson wrote: > On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg > wrote: > > > > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > > > wrote: > > > > I think instead of this we should simply make it so that the driver > > > > never tries to make the chip writable. > > > > > > I think this is a good idea, but I wasn't sure if it was an acceptable > > > behaviour change. Should the driver still try to set BCR_WPD when > > > writing an image (i.e. defer the setting of write enable until later), > > > or just not set the BCR register at all? I think your last comment was > > > the latter, but wanted to check. > > > > I would say not set it at all. I think it was (my) mistake to set it in > > the first place. > > Do you want me to remove the module parameter from intel-spi too and > do the same? No, I think that should still be left there. Then by default it is read-only and you can only enable writing if the BIOS allows it and that the user actually requested it.
Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable
On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > wrote: > > I think instead of this we should simply make it so that the driver > > never tries to make the chip writable. > > I think this is a good idea, but I wasn't sure if it was an acceptable > behaviour change. Should the driver still try to set BCR_WPD when > writing an image (i.e. defer the setting of write enable until later), > or just not set the BCR register at all? I think your last comment was > the latter, but wanted to check. I would say not set it at all. I think it was (my) mistake to set it in the first place.
Re: [PATCH] Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable
Hi,
Sorry for the delay, I was on vacation.
On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote:
> Currently, intel-spi has a module argument that controls whether the driver
> attempts to turn the SPI flash chip writeable. The default value
> is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.
>
> That's why I propose this patch to add such argument to intel-spi-pci,
> so the user can control whether the driver tries to make the chip
> writeable or not, being the default FALSE as is the argument of
> intel-spi.
>
> Signed-off-by: Daniel Gutson
> ---
> drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 81329f680bec..77e57450f166 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = {
> .type = INTEL_SPI_CNL,
> };
>
> +static bool writeable;
> +module_param(writeable, bool, 0);
> +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip
> (default=0)");
I think instead of this we should simply make it so that the driver
never tries to make the chip writable.
> +
> static int intel_spi_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> {
> @@ -41,12 +45,14 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> if (!info)
> return -ENOMEM;
>
> - /* Try to make the chip read/write */
> - pci_read_config_dword(pdev, BCR, );
> - if (!(bcr & BCR_WPD)) {
> - bcr |= BCR_WPD;
> - pci_write_config_dword(pdev, BCR, bcr);
> + if (writeable) {
> + /* Try to make the chip read/write */
> pci_read_config_dword(pdev, BCR, );
> + if (!(bcr & BCR_WPD)) {
> + bcr |= BCR_WPD;
> + pci_write_config_dword(pdev, BCR, bcr);
> + pci_read_config_dword(pdev, BCR, );
> + }
> }
> info->writeable = !!(bcr & BCR_WPD);
So here we just read the BCR register and then set info->writeable based
on its value.
Then it is up to the BIOS to enable this if it allows writing the flash
chip from the OS side.
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
On Thu, Jul 23, 2020 at 10:30:58PM +0200, Karol Herbst wrote: > On Wed, Jul 22, 2020 at 11:25 AM Mika Westerberg > wrote: > > > > On Tue, Jul 21, 2020 at 01:37:12PM -0500, Patrick Volkerding wrote: > > > On 7/21/20 10:27 AM, Mika Westerberg wrote: > > > > On Tue, Jul 21, 2020 at 11:01:55AM -0400, Lyude Paul wrote: > > > >> Sure thing. Also, feel free to let me know if you'd like access to one > > > >> of the > > > >> systems we saw breaking with this patch - I'm fairly sure I've got one > > > >> of them > > > >> locally at my apartment and don't mind setting up AMT/KVM/SSH > > > > Probably no need for remote access (thanks for the offer, though). I > > > > attached a test patch to the bug report: > > > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=208597 > > > > > > > > that tries to work it around (based on the ->pm_cap == 0). I wonder if > > > > anyone would have time to try it out. > > > > > > > > > Hi Mika, > > > > > > I can confirm that this patch applied to 5.4.52 fixes the issue with > > > hybrid graphics on the Thinkpad X1 Extreme gen2. > > > > Great, thanks for testing! > > > > yeah, works on the P1G2 as well. Thanks for testing! Since we have the revert queued for this release cycle, I think I will send an updated version of "PCI/PM: Assume ports without DLL Link Active train links in 100 ms" after v5.9-rc1 is released that has this workaround in place. (I'm continuing my vacation so will be offline next week).
Re: [PATCH] mtd: spi-nor: intel-spi: Simulate WRDI command
On Wed, Jul 22, 2020 at 03:18:50PM +, tudor.amba...@microchip.com wrote:
> On 7/22/20 5:36 PM, Mika Westerberg wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hi,
> >
> > On Wed, Jul 22, 2020 at 02:28:30PM +, tudor.amba...@microchip.com wrote:
> >> + Mika
> >>
> >> Hi, Mika,
> >>
> >> Would you please review the patch from below?
> >
> > Sure, there is minor comment below.
> >
> >>
> >> Thanks!
> >>
> >> On 7/22/20 5:01 PM, Alexander Sverdlin wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >>> the content is safe
> >>>
> >>> From: Alexander Sverdlin
> >>>
> >>> After spi_nor_write_disable() return code checks were introduced in the
> >>> spi-nor front end intel-spi backend stopped to work because WRDI was never
> >>> supported and always failed.
> >>>
> >>> Just pretend it was sucessful and ignore the command itself. HW sequencer
> >>> shall do the right thing automatically, while with SW sequencer we cannot
> >>> do it anyway, because the only tool we had was preopcode and it makes no
> >>> sense for WRDI.
> >>>
> >>> Cc: sta...@vger.kernel.org
> >>> Fixes: bce679e5ae3a ("mtd: spi-nor: Check for errors after each Register
> >>> Operation")
> >>> Signed-off-by: Alexander Sverdlin
> >>> ---
> >>> drivers/mtd/spi-nor/controllers/intel-spi.c | 8
> >>> 1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
> >>> b/drivers/mtd/spi-nor/controllers/intel-spi.c
> >>> index 61d2a0a..134b356 100644
> >>> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> >>> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> >>> @@ -612,6 +612,14 @@ static int intel_spi_write_reg(struct spi_nor *nor,
> >>> u8 opcode, const u8 *buf,
> >>> return 0;
> >>> }
> >>>
> >>> + /*
> >>> +* We hope that HW sequencer will do the right thing
> >>> automatically and
> >>> +* with the SW seuencer we cannot use preopcode any way, so just
> >>> ignore
> >
> > Typo, should be sequencer.
> >
> > Otherwise looks good to me.
> >
>
> It looks good to me too. Should I add your R-b tag when applying?
> I can fix the typo.
Sure. Thanks!
Re: [PATCH] mtd: spi-nor: intel-spi: Simulate WRDI command
Hi,
On Wed, Jul 22, 2020 at 02:28:30PM +, tudor.amba...@microchip.com wrote:
> + Mika
>
> Hi, Mika,
>
> Would you please review the patch from below?
Sure, there is minor comment below.
>
> Thanks!
>
> On 7/22/20 5:01 PM, Alexander Sverdlin wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > From: Alexander Sverdlin
> >
> > After spi_nor_write_disable() return code checks were introduced in the
> > spi-nor front end intel-spi backend stopped to work because WRDI was never
> > supported and always failed.
> >
> > Just pretend it was sucessful and ignore the command itself. HW sequencer
> > shall do the right thing automatically, while with SW sequencer we cannot
> > do it anyway, because the only tool we had was preopcode and it makes no
> > sense for WRDI.
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: bce679e5ae3a ("mtd: spi-nor: Check for errors after each Register
> > Operation")
> > Signed-off-by: Alexander Sverdlin
> > ---
> > drivers/mtd/spi-nor/controllers/intel-spi.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > index 61d2a0a..134b356 100644
> > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > @@ -612,6 +612,14 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8
> > opcode, const u8 *buf,
> > return 0;
> > }
> >
> > + /*
> > +* We hope that HW sequencer will do the right thing automatically
> > and
> > +* with the SW seuencer we cannot use preopcode any way, so just
> > ignore
Typo, should be sequencer.
Otherwise looks good to me.
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
On Tue, Jul 21, 2020 at 01:37:12PM -0500, Patrick Volkerding wrote: > On 7/21/20 10:27 AM, Mika Westerberg wrote: > > On Tue, Jul 21, 2020 at 11:01:55AM -0400, Lyude Paul wrote: > >> Sure thing. Also, feel free to let me know if you'd like access to one of > >> the > >> systems we saw breaking with this patch - I'm fairly sure I've got one of > >> them > >> locally at my apartment and don't mind setting up AMT/KVM/SSH > > Probably no need for remote access (thanks for the offer, though). I > > attached a test patch to the bug report: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=208597 > > > > that tries to work it around (based on the ->pm_cap == 0). I wonder if > > anyone would have time to try it out. > > > Hi Mika, > > I can confirm that this patch applied to 5.4.52 fixes the issue with > hybrid graphics on the Thinkpad X1 Extreme gen2. Great, thanks for testing!
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
On Tue, Jul 21, 2020 at 02:24:19PM -0400, Lyude Paul wrote: > On Tue, 2020-07-21 at 12:00 -0400, Lyude Paul wrote: > > On Tue, 2020-07-21 at 18:27 +0300, Mika Westerberg wrote: > > > On Tue, Jul 21, 2020 at 11:01:55AM -0400, Lyude Paul wrote: > > > > Sure thing. Also, feel free to let me know if you'd like access to one > > > > of > > > > the > > > > systems we saw breaking with this patch - I'm fairly sure I've got one > > > > of > > > > them > > > > locally at my apartment and don't mind setting up AMT/KVM/SSH > > > > > > Probably no need for remote access (thanks for the offer, though). I > > > attached a test patch to the bug report: > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=208597 > > > > > > that tries to work it around (based on the ->pm_cap == 0). I wonder if > > > anyone would have time to try it out. > > > > Will give it a shot today and let you know the result > > Ahh-actually, I thought the laptop I had locally could reproduce this bug but > that doesn't appear to be the case whoops. Karol Herbst still has access to a > machine that can test this though, so they'll likely get to trying the patch > today or tommorrow OK sounds good :)
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
On Tue, Jul 21, 2020 at 11:01:55AM -0400, Lyude Paul wrote: > Sure thing. Also, feel free to let me know if you'd like access to one of the > systems we saw breaking with this patch - I'm fairly sure I've got one of them > locally at my apartment and don't mind setting up AMT/KVM/SSH Probably no need for remote access (thanks for the offer, though). I attached a test patch to the bug report: https://bugzilla.kernel.org/show_bug.cgi?id=208597 that tries to work it around (based on the ->pm_cap == 0). I wonder if anyone would have time to try it out.
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
On Fri, Jul 17, 2020 at 09:52:09PM +0200, Lukas Wunner wrote: > On Fri, Jul 17, 2020 at 03:04:10PM -0400, Lyude Paul wrote: > > Isn't it possible to tell whether a PCI device is connected through > > thunderbolt or not? We could probably get away with just defaulting > > to 100ms for thunderbolt devices without DLL Link Active specified, > > and then default to the old delay value for non-thunderbolt devices. > > pci_is_thunderbolt_attached() That only works with some devices. I think we should try to keep the fact that some PCIe links may be tunneled over TBT/USB4 transparent to the PCI core and try to treat them as "standard" PCIe links if possible at all :)
Re: nouveau regression with 5.7 caused by "PCI/PM: Assume ports without DLL Link Active train links in 100 ms"
Hi, [Sorry for the delay, I was on vacation] On Fri, Jul 17, 2020 at 01:32:10PM +0200, Karol Herbst wrote: > Filed at https://bugzilla.kernel.org/show_bug.cgi?id=208597 Thanks for reporting. I'll check your logs and try to figure if there is something we can do to make both nouveau and TBT working at the same time. > oddly enough I wasn't able to reproduce it on my XPS 9560, will ping > once something breaks.
Re: [PATCH v3 0/2] Allow breaking up Thunderbolt/USB4 updates
On Tue, Jun 23, 2020 at 11:14:27AM -0500, Mario Limonciello wrote: > Currently updates to Thunderbolt and USB4 controllers are fully atomic > actions. When writing into the non-active NVM nothing gets flushed to > the hardware until authenticate is sent. > > There has been some desire to improve the perceived performance of these > updates, particularly for userland that may perform the update upon > a performance sensitive time like logging out. > > So allow userland to flush the image to hardware at runtime, and then > allow authenticating the image at another time. > > For the Dell WD19TB some specific hardware capability exists that allows > extending this to automatically complete the update when unplugged. > Export that functionality to userspace as well. > > Changes from v2 to v3: > - Correct some whitespace and kernel-doc comments > - Add another missing 'const' > - For a quirk: (1<<1) -> BIT(0) > > Changes from v1 to v2: > - Improve documentation > - Drop tb-quirks.h > - Adjust function and parameter names to Mika's preferences > - Rebase onto thunderbolt.git/bleeding-edge to move on top of retimer work > > Mario Limonciello (2): > thunderbolt: Add support for separating the flush to SPI and > authenticate > thunderbolt: Add support for authenticate on disconnect Both applied to thunderbolt.git/next, thanks Mario!
Re: [PATCH][next] thunderbolt: ensure left shift of 512 does not overflow a 32 bit int
On Tue, Jun 30, 2020 at 03:55:58PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The 32 bit int value 512 is being left shifted and then used in a context
> that expects the expression to be a larger unsigned long. There may be
> a potential integer overflow, so make 512 a UL before shift to avoid
> any such issues.
>
> Addresses-Coverity: ("Uninintentional integer overflow")
> Fixes: 3b1d8d577ca8 ("thunderbolt: Implement USB3 bandwidth negotiation
> routines")
> Signed-off-by: Colin Ian King
Applied, thanks!
Re: [PATCH] x86/resource: Do not exclude regions that are marked as MMIO in EFI memmap
On Fri, Jun 26, 2020 at 05:43:44PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 02, 2020 at 05:14:51PM +0300, Mika Westerberg wrote:
> > Commit 4dc2287c1805 ("x86: avoid E820 regions when allocating address
> > space") made the resource allocation code to avoid all regions that are
> > in E820 table. This prevents the kernel to assign MMIO resources to
> > regions that may be real RAM for example.
> >
> > However, at least with Lenovo Yoca C940 and S740 this causes problems
> > when allocating resources for PCIe devices behind Thunderbolt port(s).
> >
> > On Yoga S740 the E820 table contains an entry like this:
> >
> > BIOS-e820: [mem 0x2bc5-0xcfff] reserved
> >
> > and ACPI _CRS method for the host bridge returns these windows:
> >
> > pci_bus :00: root bus resource [io 0x-0x0cf7 window]
> > pci_bus :00: root bus resource [io 0x0d00-0x window]
> > pci_bus :00: root bus resource [mem 0x000a-0x000b window]
> > pci_bus :00: root bus resource [mem 0x4540-0xbfff window]
> > pci_bus :00: root bus resource [mem 0x40-0x7f window]
> >
> > Note that the 0x4540-0xbfff entry is also included in the E820
> > table and marked as "reserved".
> >
> > When Thunderbolt device is connected and the PCIe gets tunneled PCI core
> > tries to allocate memory for the new devices but it fails because all
> > the resources are inside this reserved region so arch_remove_reservations()
> > clips them which makes the resource assignment fail as in below log:
> >
> > pci :00:07.0: PCI bridge to [bus 01-2a]
> > pci :00:07.0: bridge window [mem 0x4600-0x521f]
> > pci :00:07.0: bridge window [mem 0x60-0x601bff 64bit
> > pref]
> > ...
> > pci :02:04.0: bridge window [mem 0x0010-0x001f 64bit pref] to
> > [bus 07-2a] add_size 10 add_align 10
> > pci :02:04.0: bridge window [mem 0x0010-0x001f] to [bus
> > 07-2a] add_size 10 add_align 10
> > pci :01:00.0: bridge window [mem 0x0010-0x005f 64bit pref] to
> > [bus 02-2a] add_size 10 add_align 10
> > pci :01:00.0: bridge window [mem 0x0010-0x005f] to [bus
> > 02-2a] add_size 10 add_align 10
> > pci :01:00.0: bridge window [io 0x1000-0x5fff] shrunken by
> > 0x4000
> > pci :01:00.0: bridge window [mem 0x0010-0x005f] extended by
> > 0x0bd0
> > pci :01:00.0: bridge window [mem 0x0010-0x005f 64bit pref]
> > extended by 0x1bb0
> > pci :02:04.0: bridge window [mem 0x0010-0x001f] extended by
> > 0x0bd0
> > pci :02:04.0: bridge window [mem 0x0010-0x001f 64bit pref]
> > extended by 0x1bb0
> > pci :01:00.0: BAR 8: no space for [mem size 0x0c20]
> > pci :01:00.0: BAR 8: failed to assign [mem size 0x0c20]
> > pci :01:00.0: BAR 9: assigned [mem 0x60-0x601bff 64bit
> > pref]
> > pci :01:00.0: BAR 7: assigned [io 0x4000-0x4fff]
> >
> > The 01:00.0 is the upstream port of the PCIe switch that is connected to
> > the PCIe root port (00:07.1) over Thunderbolt link.
> >
> > If I add "efi=debug" to the command line I can see that the EFI memory
> > map actually contains several entries:
> >
> > [Reserved | | | | | | | | | |WB|WT|WC|UC]
> > range=[0x2bc5-0x3fff] (323MB)
> > [Reserved | | | | | | | | | |WB| | |UC]
> > range=[0x4000-0x40ff] (16MB)
> > [Reserved | | | | | | | | | | | | | ]
> > range=[0x4100-0x453f] (68MB)
> > [Memory Mapped I/O |RUN| | | | | | | | | | | |UC]
> > range=[0x4540-0xcfff] (2220MB)
> >
> > I think the EFI stub merges these consecutive entries into that single
> > E820 entry showed above. The last region marked as EFI_MEMORY_MAPPED_IO
> > actually covers the PCI host bridge window entirely. However, since
> > there is corresponding E820 type for this it is simply marked as
> > E820_TYPE_RESERVED.
>
> Is this supposed to say "since there is *no* corresponding E820 type"?
Yes, I think it makes more sense that way :)
> I guess this refers to setup_e820()? I see the switch case there that
> sets e820_type to E820_TYPE_RESERVED for EFI_MEMORY_MAPPED_IO.
>
> I'm really not familia
Re: linux-next: build failure after merge of the thunderbolt tree
On Tue, Jun 30, 2020 at 04:03:46PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the thunderbolt tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
>
> Caused by commit
>
> 54509f5005ca ("thunderbolt: Add KUnit tests for path walking")
>
> interacting with commit
>
> d4cdd146d0db ("kunit: generalize kunit_resource API beyond allocated
> resources")
>
> from the kunit-next tree.
Thanks for reporting and fixing. The fix looks good to me.
Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support
On Thu, Jun 25, 2020 at 01:30:53PM +0200, Rafael J. Wysocki wrote: > It is not necessary to call acpi_pci_find_companion() from > pci_acpi_storage_d3() as long as that function is required to be > called by the target device's driver probe or later. > > Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it > is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(), > and that gets called from pci_device_add() *before* calling > device_add(). > > Mika, is that why acpi_pci_find_companion() gets callled from > acpi_pci_bridge_d3()? Yes, that's correct.
Re: [PATCH v2 2/2] thunderbolt: Add support for authenticate on disconnect
On Mon, Jun 22, 2020 at 01:57:58PM -0500, Mario Limonciello wrote:
> Some external devices can support completing thunderbolt authentication
> when they are unplugged. For this to work though, the link controller must
> remain operational.
>
> The only device known to support this right now is the Dell WD19TB, so add
> a quirk for this.
>
> Signed-off-by: Mario Limonciello
> ---
> .../ABI/testing/sysfs-bus-thunderbolt | 13 ++
> drivers/thunderbolt/Makefile | 2 +-
> drivers/thunderbolt/eeprom.c | 1 +
> drivers/thunderbolt/lc.c | 14 +++
> drivers/thunderbolt/quirks.c | 36 +
> drivers/thunderbolt/switch.c | 40 +--
> drivers/thunderbolt/tb.h | 7
> drivers/thunderbolt/tb_regs.h | 1 +
> 8 files changed, 109 insertions(+), 5 deletions(-)
> create mode 100644 drivers/thunderbolt/quirks.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 7d0500b4d58a..dd565c378b40 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -276,3 +276,16 @@ Date: Oct 2020
> KernelVersion: v5.9
> Contact: Mika Westerberg
> Description: Retimer vendor identifier read from the hardware.
> +
> +What:
> /sys/bus/thunderbolt/devices/.../nvm_authenticate_on_disconnect
> +Date:Oct 2020
> +KernelVersion: v5.9
> +Contact: Mario Limonciello
> +Description: For supported devices, automatically authenticate the new
> Thunderbolt
> + image when the device is disconnected from the host system.
> +
> + This file will accept writing values "1" or "2"
> + - Writing "1" will flush the image to the storage
> + area and prepare the device for authentication on disconnect.
> + - Writing "2" will run some basic validation on the image
> + and flush it to the storage area.
> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
> index cf7e1b42f4ad..4ab5bfad7bfd 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -2,6 +2,6 @@
> obj-${CONFIG_USB4} := thunderbolt.o
> thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o path.o
> tunnel.o eeprom.o
> thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o
> tmu.o usb4.o
> -thunderbolt-objs += nvm.o retimer.o
> +thunderbolt-objs += nvm.o retimer.o quirks.o
>
> obj-${CONFIG_USB4_KUNIT_TEST} += test.o
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index b451a5aa90b5..3ebca44ab3fa 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -599,6 +599,7 @@ int tb_drom_read(struct tb_switch *sw)
> sw->uid = header->uid;
> sw->vendor = header->vendor_id;
> sw->device = header->model_id;
> + tb_check_quirks(sw);
>
> crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
> if (crc != header->data_crc32) {
> diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
> index bd44d50246d2..828b4655d6a1 100644
> --- a/drivers/thunderbolt/lc.c
> +++ b/drivers/thunderbolt/lc.c
> @@ -366,3 +366,17 @@ int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct
> tb_port *in)
> tb_port_dbg(in, "sink %d de-allocated\n", sink);
> return 0;
> }
> +
> +/**
> + * tb_lc_force_power() - Forces LC to be powered on
> + * @sw: thunderbolt switch
@sw: Thunderbolt switch
with capital T.
> + *
> + * This is useful to let authentication cycle pass even without
> + * a Thunderbolt link present.
> + */
> +int tb_lc_force_power(struct tb_switch *sw)
> +{
> + u32 in = 0x;
> +
> + return tb_sw_write(sw, , TB_CFG_SWITCH, TB_LC_POWER, 1);
> +}
> diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
> new file mode 100644
> index ..e8eace99bfcb
> --- /dev/null
> +++ b/drivers/thunderbolt/quirks.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thunderbolt driver - quirks
> + *
> + * Copyright (c) 2020 Mario Limonciello
> + */
> +
> +#include "tb.h"
> +
> +static void quirk_force_power_link(struct tb_switch *sw)
> +{
> + sw->quirks |= QUIRK_FORCE_POWER_LINK_CONTROLLER;
> +}
> +
> +struct tb_quirk {
> + u16 vendor;
> + u16 device;
> + void (*h
Re: [PATCH v2 1/2] thunderbolt: Add support for separating the flush to SPI and authenticate
On Mon, Jun 22, 2020 at 01:57:57PM -0500, Mario Limonciello wrote:
> This allows userspace to have a shorter period of time that the device
> is unusable and to call it at a more convenient time.
>
> For example flushing the image may happen while the user is using the
> machine and authenticating/rebooting may happen while logging out.
>
> Signed-off-by: Mario Limonciello
> ---
> .../ABI/testing/sysfs-bus-thunderbolt | 11 -
> drivers/thunderbolt/nvm.c | 1 +
> drivers/thunderbolt/switch.c | 42 ---
> drivers/thunderbolt/tb.h | 2 +
> 4 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index bd504ed323e8..7d0500b4d58a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -178,11 +178,18 @@ KernelVersion: 4.13
> Contact: thunderbolt-softw...@lists.01.org
> Description: When new NVM image is written to the non-active NVM
> area (through non_activeX NVMem device), the
> - authentication procedure is started by writing 1 to
> - this file. If everything goes well, the device is
> + authentication procedure is started by writing to
> + this file.
> + If everything goes well, the device is
> restarted with the new NVM firmware. If the image
> verification fails an error code is returned instead.
>
> + This file will accept writing values "1" or "2"
> + - Writing "1" will flush the image to the storage
> + area and authenticate the image in one action.
> + - Writing "2" will run some basic validation on the image
> + and flush it to the storage area.
> +
> When read holds status of the last authentication
> operation if an error occurred during the process. This
> is directly the status value from the DMA configuration
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index 4c6aa06ab3d5..29de6d95c6e7 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -100,6 +100,7 @@ int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int
> offset, void *val,
> return -ENOMEM;
> }
>
> + nvm->flushed = false;
This means every write invalidates the "flushed" state, right?
> nvm->buf_data_size = offset + bytes;
> memcpy(nvm->buf + offset, val, bytes);
> return 0;
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 817c66c7adcf..bbfbfebeee7f 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -26,6 +26,11 @@ struct nvm_auth_status {
> u32 status;
> };
>
> +enum nvm_write_ops {
> + WRITE_AND_AUTHENTICATE = 1,
> + WRITE_ONLY = 2,
> +};
> +
> /*
> * Hold NVM authentication failure status per switch This information
> * needs to stay around even when the switch gets power cycled so we
> @@ -155,8 +160,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
> }
>
> if (tb_switch_is_usb4(sw))
> - return usb4_switch_nvm_write(sw, 0, buf, image_size);
> - return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> + else
> + ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + if (!ret)
> + sw->nvm->flushed = true;
> + return ret;
> }
>
> static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
> @@ -1488,7 +1497,7 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> struct tb_switch *sw = tb_to_switch(dev);
> - bool val;
> + int val;
> int ret;
>
> pm_runtime_get_sync(>dev);
> @@ -1504,25 +1513,28 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> goto exit_unlock;
> }
>
> - ret = kstrtobool(buf, );
> + ret = kstrtoint(buf, 10, );
> if (ret)
> goto exit_unlock;
>
> /* Always clear the authentication status */
> nvm_clear_auth_status(sw);
>
> - if (val) {
> - if (!sw->nvm->buf) {
> - ret = -EINVAL;
> - goto exit_unlock;
> - }
> -
> - ret = nvm_validate_and_write(sw);
> - if (ret)
> - goto exit_unlock;
> + if (val > 0) {
> + if (!sw->nvm->flushed) {
> + if (!sw->nvm->buf) {
> + ret = -EINVAL;
> + goto exit_unlock;
> + }
>
> - sw->nvm->authenticating = true;
> - ret
Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates
On Mon, Jun 22, 2020 at 04:41:35PM +, mario.limoncie...@dell.com wrote: > > -Original Message- > > From: Mika Westerberg > > Sent: Monday, June 22, 2020 11:38 AM > > To: Limonciello, Mario > > Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; > > linux-...@vger.kernel.org; > > linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates > > > > > > [EXTERNAL EMAIL] > > > > Hi Mario, > > > > On Mon, Jun 22, 2020 at 09:30:33AM -0500, Mario Limonciello wrote: > > > Currently updates to Thunderbolt and USB4 controllers are fully atomic > > > actions. When writing into the non-active NVM nothing gets flushed to > > > the hardware until authenticate is sent. > > > > > > There has been some desire to improve the perceived performance of these > > > updates, particularly for userland that may perform the update upon > > > a performance sensitive time like logging out. > > > > > > So allow userland to flush the image to hardware at runtime, and then > > > allow authenticating the image at another time. > > > > > > For the Dell WD19TB some specific hardware capability exists that allows > > > extending this to automatically complete the update when unplugged. > > > Export that functionality to userspace as well. > > > > > > This patch series is done relative thunderbolt.git/next. > > > > Thanks for the patch series. I wonder if you could base this on top of > > my "retimer NVM upgrade" series here (you are also Cc'd): > > > > https://lore.kernel.org/linux-usb/20200616135617.85752-1- > > mika.westerb...@linux.intel.com/ > > > > That series moves some of the common NVM functionality into a separate > > file (nvm.c). > > Sure thing. Do you by chance already have that on a public branch somewhere > that I can easily rebase it? I just pushed "bleeding-edge" branch that you should be able to base your stuff on top: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git/log/?h=bleeding-edge It includes a couple of other patches as well (subject to change since they are also under review).
Re: [PATCH 2/2] thunderbolt: Add support for authenticate on disconnect
On Mon, Jun 22, 2020 at 09:30:35AM -0500, Mario Limonciello wrote:
> Some external devices can support completing thunderbolt authentication
> when they are unplugged. For this to work though, the link controller must
> remain operational.
>
> The only device known to support this right now is the Dell WD19TB, so add
> a quirk for this.
>
> Signed-off-by: Mario Limonciello
> ---
> .../ABI/testing/sysfs-bus-thunderbolt | 13 ++
> drivers/thunderbolt/Makefile | 1 +
> drivers/thunderbolt/eeprom.c | 2 +
> drivers/thunderbolt/lc.c | 14 +++
> drivers/thunderbolt/quirks.c | 38 +
> drivers/thunderbolt/switch.c | 42 +--
> drivers/thunderbolt/tb-quirks.h | 16 +++
> drivers/thunderbolt/tb.h | 3 ++
> drivers/thunderbolt/tb_regs.h | 1 +
> 9 files changed, 126 insertions(+), 4 deletions(-)
> create mode 100644 drivers/thunderbolt/quirks.c
> create mode 100644 drivers/thunderbolt/tb-quirks.h
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 26b15cbc9881..30798f9a8f59 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -243,3 +243,16 @@ KernelVersion: 4.15
> Contact: thunderbolt-softw...@lists.01.org
> Description: This contains XDomain service specific settings as
> bitmask. Format: %x
> +
> +What:
> /sys/bus/thunderbolt/devices/.../nvm_authenticate_on_disconnect
> +Date:August 2020
> +KernelVersion: 5.9
> +Contact: thunderbolt-softw...@lists.01.org
I think you can use your email address here instead.
> +Description: For supported devices, automatically authenticate the new
> Thunderbolt
> + image when the device is disconnected from the host system.
> +
> + This file will accept writing values "1" or "2"
> + - Writing "1" will flush the image to the storage
> + area and prepare the device for authentication on disconnect.
> + - Writing "2" will only flush the image to the storage
> + area.
Also here it does the basic image validation so probably good to
mention.
> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
> index eae28dd45250..013c5564565a 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -2,3 +2,4 @@
> obj-${CONFIG_USB4} := thunderbolt.o
> thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o path.o
> tunnel.o eeprom.o
> thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o
> tmu.o usb4.o
> +thunderbolt-objs += quirks.o
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index b451a5aa90b5..32838c016c4f 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include "tb.h"
> +#include "tb-quirks.h"
>
> /**
> * tb_eeprom_ctl_write() - write control word
> @@ -599,6 +600,7 @@ int tb_drom_read(struct tb_switch *sw)
> sw->uid = header->uid;
> sw->vendor = header->vendor_id;
> sw->device = header->model_id;
> + check_tbt_quirks(sw);
>
> crc = tb_crc32(sw->drom + TB_DROM_DATA_START, header->data_len);
> if (crc != header->data_crc32) {
> diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c
> index bd44d50246d2..45d2a1de2069 100644
> --- a/drivers/thunderbolt/lc.c
> +++ b/drivers/thunderbolt/lc.c
> @@ -366,3 +366,17 @@ int tb_lc_dp_sink_dealloc(struct tb_switch *sw, struct
> tb_port *in)
> tb_port_dbg(in, "sink %d de-allocated\n", sink);
> return 0;
> }
> +
> +/**
> + * lc_force_power() - Force powers a ridge
Maybe "Forces LC to be powered on" or similar?
> + * @sw: thunderbolt switch
> + *
> + * This is useful to let authentication cycle pass even without
> + * a Thunderbolt link present
Add "." at the end of the sentence.
> + */
> +int lc_force_power(struct tb_switch *sw)
Also tb_lc_force_power() or so to follow the conventions used in the
driver.
> +{
> + u32 in = 0x;
I prefer 0x instead.
> +
> + return tb_sw_write(sw, , TB_CFG_SWITCH, TB_LC_POWER, 1);
> +}
> diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
> new file mode 100644
> index ..b9c5cfb97645
> --- /dev/null
> +++ b/drivers/thunderbolt/quirks.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thunderbolt driver - quirks
> + *
> + * Copyright (c) 2020 Mario Limonciello
> + */
> +
> +#include "tb.h"
> +#include "tb-quirks.h"
> +
> +static void quirk_force_power_link(struct tb_switch *sw)
> +{
> + sw->quirks |= QUIRK_FORCE_POWER_LINK_CONTROLLER;
> +}
> +
> +struct tbt_quirk {
tb_quirk please.
> + u16 vendor;
> +
Re: [PATCH 1/2] thunderbolt: Add support for separating the flush to SPI and authenticate
On Mon, Jun 22, 2020 at 09:30:34AM -0500, Mario Limonciello wrote:
> This allows userspace to have a shorter period of time that the device
> is unusable and to call it at a more convenient time.
>
> For example flushing the image may happen while the user is using the
> machine and authenticating/rebooting may happen while logging out.
>
> Signed-off-by: Mario Limonciello
> ---
> .../ABI/testing/sysfs-bus-thunderbolt | 11 -
> drivers/thunderbolt/switch.c | 43 ---
> drivers/thunderbolt/tb.h | 1 +
> 3 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index 82e80de78dd0..26b15cbc9881 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -178,11 +178,18 @@ KernelVersion: 4.13
> Contact: thunderbolt-softw...@lists.01.org
> Description: When new NVM image is written to the non-active NVM
> area (through non_activeX NVMem device), the
> - authentication procedure is started by writing 1 to
> - this file. If everything goes well, the device is
> + authentication procedure is started by writing to
> + this file.
> + If everything goes well, the device is
> restarted with the new NVM firmware. If the image
> verification fails an error code is returned instead.
>
> + This file will accept writing values "1" or "2"
> + - Writing "1" will flush the image to the storage
> + area and authenticate the image in one action.
> + - Writing "2" will only flush the image to the storage
> + area.
Does this ("2") also do the basic validation? I think it does so
probably good to mention that here.
> +
> When read holds status of the last authentication
> operation if an error occurred during the process. This
> is directly the status value from the DMA configuration
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index d7d60cd9226f..4c476a58db38 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -27,6 +27,11 @@
> #define NVM_MIN_SIZE SZ_32K
> #define NVM_MAX_SIZE SZ_512K
>
> +enum nvm_write_ops {
> + WRITE_AND_AUTHENTICATE = 1,
> + WRITE_ONLY = 2,
> +};
> +
> static DEFINE_IDA(nvm_ida);
>
> struct nvm_auth_status {
> @@ -164,8 +169,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
> }
>
> if (tb_switch_is_usb4(sw))
> - return usb4_switch_nvm_write(sw, 0, buf, image_size);
> - return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> + else
> + ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + if (!ret)
> + sw->nvm->flushed = true;
> + return ret;
> }
>
> static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
> @@ -371,6 +380,7 @@ static int tb_switch_nvm_write(void *priv, unsigned int
> offset, void *val,
> }
> }
>
> + sw->nvm->flushed = false;
> sw->nvm->buf_data_size = offset + bytes;
> memcpy(sw->nvm->buf + offset, val, bytes);
>
> @@ -1536,7 +1546,7 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> struct tb_switch *sw = tb_to_switch(dev);
> - bool val;
> + int val;
> int ret;
>
> pm_runtime_get_sync(>dev);
> @@ -1552,25 +1562,28 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> goto exit_unlock;
> }
>
> - ret = kstrtobool(buf, );
> + ret = kstrtoint(buf, 10, );
> if (ret)
> goto exit_unlock;
>
> /* Always clear the authentication status */
> nvm_clear_auth_status(sw);
>
> - if (val) {
> - if (!sw->nvm->buf) {
> - ret = -EINVAL;
> - goto exit_unlock;
> - }
> -
> - ret = nvm_validate_and_write(sw);
> - if (ret)
> - goto exit_unlock;
> + if (val > 0) {
> + if (!sw->nvm->flushed) {
> + if (!sw->nvm->buf) {
> + ret = -EINVAL;
> + goto exit_unlock;
> + }
>
> - sw->nvm->authenticating = true;
> - ret = nvm_authenticate(sw);
> + ret = nvm_validate_and_write(sw);
> + if (ret || val == WRITE_ONLY)
> + goto exit_unlock;
> + }
> + if (val == WRITE_AND_AUTHENTICATE) {
> + sw->nvm->authenticating =
Re: [PATCH 0/2] Allow breaking up Thunderbolt/USB4 updates
Hi Mario, On Mon, Jun 22, 2020 at 09:30:33AM -0500, Mario Limonciello wrote: > Currently updates to Thunderbolt and USB4 controllers are fully atomic > actions. When writing into the non-active NVM nothing gets flushed to > the hardware until authenticate is sent. > > There has been some desire to improve the perceived performance of these > updates, particularly for userland that may perform the update upon > a performance sensitive time like logging out. > > So allow userland to flush the image to hardware at runtime, and then > allow authenticating the image at another time. > > For the Dell WD19TB some specific hardware capability exists that allows > extending this to automatically complete the update when unplugged. > Export that functionality to userspace as well. > > This patch series is done relative thunderbolt.git/next. Thanks for the patch series. I wonder if you could base this on top of my "retimer NVM upgrade" series here (you are also Cc'd): https://lore.kernel.org/linux-usb/20200616135617.85752-1-mika.westerb...@linux.intel.com/ That series moves some of the common NVM functionality into a separate file (nvm.c). > Mario Limonciello (2): > thunderbolt: Add support for separating the flush to SPI and > authenticate > thunderbolt: Add support for authenticate on disconnect > > .../ABI/testing/sysfs-bus-thunderbolt | 24 +- > drivers/thunderbolt/Makefile | 1 + > drivers/thunderbolt/eeprom.c | 2 + > drivers/thunderbolt/lc.c | 14 > drivers/thunderbolt/quirks.c | 38 + > drivers/thunderbolt/switch.c | 81 +++ > drivers/thunderbolt/tb-quirks.h | 16 > drivers/thunderbolt/tb.h | 4 + > drivers/thunderbolt/tb_regs.h | 1 + > 9 files changed, 162 insertions(+), 19 deletions(-) > create mode 100644 drivers/thunderbolt/quirks.c > create mode 100644 drivers/thunderbolt/tb-quirks.h > > -- > 2.25.1
[PATCH RESEND] x86/resource: Do not exclude regions that are marked as MMIO in EFI memmap
Commit 4dc2287c1805 ("x86: avoid E820 regions when allocating address
space") made the resource allocation code to avoid all regions that are
in E820 table. This prevents the kernel to assign MMIO resources to
regions that may be real RAM for example.
However, at least with Lenovo Yoca C940 and S740 this causes problems
when allocating resources for PCIe devices behind Thunderbolt port(s).
On Yoga S740 the E820 table contains an entry like this:
BIOS-e820: [mem 0x2bc5-0xcfff] reserved
and ACPI _CRS method for the host bridge returns these windows:
pci_bus :00: root bus resource [io 0x-0x0cf7 window]
pci_bus :00: root bus resource [io 0x0d00-0x window]
pci_bus :00: root bus resource [mem 0x000a-0x000b window]
pci_bus :00: root bus resource [mem 0x4540-0xbfff window]
pci_bus :00: root bus resource [mem 0x40-0x7f window]
Note that the 0x4540-0xbfff entry is also included in the E820
table and marked as "reserved".
When Thunderbolt device is connected and the PCIe gets tunneled PCI core
tries to allocate memory for the new devices but it fails because all
the resources are inside this reserved region so arch_remove_reservations()
clips them which makes the resource assignment fail as in below log:
pci :00:07.0: PCI bridge to [bus 01-2a]
pci :00:07.0: bridge window [mem 0x4600-0x521f]
pci :00:07.0: bridge window [mem 0x60-0x601bff 64bit pref]
...
pci :02:04.0: bridge window [mem 0x0010-0x001f 64bit pref] to
[bus 07-2a] add_size 10 add_align 10
pci :02:04.0: bridge window [mem 0x0010-0x001f] to [bus 07-2a]
add_size 10 add_align 10
pci :01:00.0: bridge window [mem 0x0010-0x005f 64bit pref] to
[bus 02-2a] add_size 10 add_align 10
pci :01:00.0: bridge window [mem 0x0010-0x005f] to [bus 02-2a]
add_size 10 add_align 10
pci :01:00.0: bridge window [io 0x1000-0x5fff] shrunken by
0x4000
pci :01:00.0: bridge window [mem 0x0010-0x005f] extended by
0x0bd0
pci :01:00.0: bridge window [mem 0x0010-0x005f 64bit pref]
extended by 0x1bb0
pci :02:04.0: bridge window [mem 0x0010-0x001f] extended by
0x0bd0
pci :02:04.0: bridge window [mem 0x0010-0x001f 64bit pref]
extended by 0x1bb0
pci :01:00.0: BAR 8: no space for [mem size 0x0c20]
pci :01:00.0: BAR 8: failed to assign [mem size 0x0c20]
pci :01:00.0: BAR 9: assigned [mem 0x60-0x601bff 64bit pref]
pci :01:00.0: BAR 7: assigned [io 0x4000-0x4fff]
The 01:00.0 is the upstream port of the PCIe switch that is connected to
the PCIe root port (00:07.1) over Thunderbolt link.
If I add "efi=debug" to the command line I can see that the EFI memory
map actually contains several entries:
[Reserved | | | | | | | | | |WB|WT|WC|UC]
range=[0x2bc5-0x3fff] (323MB)
[Reserved | | | | | | | | | |WB| | |UC]
range=[0x4000-0x40ff] (16MB)
[Reserved | | | | | | | | | | | | | ]
range=[0x4100-0x453f] (68MB)
[Memory Mapped I/O |RUN| | | | | | | | | | | |UC]
range=[0x4540-0xcfff] (2220MB)
I think the EFI stub merges these consecutive entries into that single
E820 entry showed above. The last region marked as EFI_MEMORY_MAPPED_IO
actually covers the PCI host bridge window entirely. However, since
there is corresponding E820 type for this it is simply marked as
E820_TYPE_RESERVED.
All in all, I think we can fix this by modifying arch_remove_reservations()
to check the EFI type as well and if it is EFI_MEMORY_MAPPED_IO skip the
clipping in that case.
Reported-by: Benoit Grégoire
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206459
Signed-off-by: Mika Westerberg
---
arch/x86/kernel/resource.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/resource.c b/arch/x86/kernel/resource.c
index 9b9fb7882c20..c0bc9117dd7d 100644
--- a/arch/x86/kernel/resource.c
+++ b/arch/x86/kernel/resource.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+#include
#include
#include
@@ -36,6 +37,36 @@ static void remove_e820_regions(struct resource *avail)
}
}
+#ifdef CONFIG_EFI
+static bool efi_mmio_mem(const struct resource *avail)
+{
+ resource_size_t start, end;
+ efi_memory_desc_t desc;
+
+ if (!efi_enabled(EFI_MEMMAP) ||
+ efi_mem_desc_lookup(avail->start, ))
+ return false;
+
+ start = desc.phys_addr;
+ end = desc.phys_addr + (desc.num_pages << EFI_PAGE_SHIFT) - 1;
+
+ /*
+* No need to clip the resource if it is fully conta
Re: [PATCH 2/4] pci: set "untrusted" flag for truly external devices only
On Mon, Jun 15, 2020 at 06:17:40PM -0700, Rajat Jain wrote: > The "ExternalFacing" devices (root ports) are still internal devices > that sit on the internal system fabric and thus trusted. Currently they > were being marked untrusted - likely as an unintended border case. It was actually intentional :) At the time this was added we did not see benefits from doing this and even with this you actually are going to still miss things like a TBT chip that is soldered on the motherboard, I guess that can be though as an internal device as well. No objections to this patch, though.
Re: [PATCH] thunderbolt: Improve USB4 config symbol help text
On Tue, Jun 02, 2020 at 02:28:15PM +0200, Geert Uytterhoeven wrote:
> Fix the spelling of "specification", and add a missing "the" article.
>
> Fixes: 690ac0d20d4022bb ("thunderbolt: Update Kconfig entries to USB4")
> Signed-off-by: Geert Uytterhoeven
Applied to thunderbolt.git/next, thanks!
Re: [PATCH] mtd: revert "spi-nor: intel: provide a range for poll_timout"
On Wed, Jun 10, 2020 at 10:46:49PM +, Luis Alberto Herrera wrote: > This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range > for poll_timout". That change introduces a performance regression when > reading sequentially from flash. Logging calls to intel_spi_read without > this change we get: > > Start MTD read > [ 20.045527] intel_spi_read(from=180, len=40) > [ 20.045527] intel_spi_read(from=180, len=40) > [ 282.199274] intel_spi_read(from=1c0, len=40) > [ 282.199274] intel_spi_read(from=1c0, len=40) > [ 544.351528] intel_spi_read(from=200, len=40) > [ 544.351528] intel_spi_read(from=200, len=40) > End MTD read > > With this change: > > Start MTD read > [ 21.942922] intel_spi_read(from=1c0, len=40) > [ 21.942922] intel_spi_read(from=1c0, len=40) > [ 23.784058] intel_spi_read(from=200, len=40) > [ 23.784058] intel_spi_read(from=200, len=40) > [ 25.625006] intel_spi_read(from=240, len=40) > [ 25.625006] intel_spi_read(from=240, len=40) > End MTD read > > Signed-off-by: Luis Alberto Herrera Acked-by: Mika Westerberg
