* Re: [PATCH 2/2] clk: tegra: Always program PLL_E when enabled
From: LABBE Corentin @ 2020-06-03 13:13 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Stephen Boyd, Jon Hunter, Dmitry Osipenko,
linux-clk, linux-tegra
In-Reply-To: <20200603111923.3545261-2-thierry.reding@gmail.com>
On Wed, Jun 03, 2020 at 01:19:23PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Commit bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> added checks to avoid enabling PLLs that have already been enabled by
> the bootloader. However, the PLL_E configuration inherited from the
> bootloader isn't necessarily the one that is needed for the kernel.
>
> This can cause SATA to fail like this:
>
> [ 5.310270] phy phy-sata.6: phy poweron failed --> -110
> [ 5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: -110
> [ 5.323022] tegra-ahci: probe of 70027000.sata failed with error -110
>
> Fix this by always programming the PLL_E. This ensures that any mis-
> configuration by the bootloader will be overwritten by the kernel.
>
> Fixes: bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
> Reported-by: LABBE Corentin <clabbe@baylibre.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> drivers/clk/tegra/clk-pll.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 583d2ac61e9e..b2d39a66f0fa 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1601,9 +1601,6 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
> unsigned long flags = 0;
> unsigned long input_rate;
>
> - if (clk_pll_is_enabled(hw))
> - return 0;
> -
> input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
>
> if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
> --
> 2.24.1
>
Tested-by: Corentin Labbe <clabbe@baylibre.com>
Thanks
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-06-03 13:07 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
patches-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA, Rafael J . Wysocki,
Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <8f97d2e1-497a-495d-bc82-f46dbeba440c-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi Hans, hi Dmitry,
W dniu 02.06.2020 o 22:19, Hans de Goede pisze:
> Hi,
>
> On 6/2/20 8:50 PM, Andrzej Pietrasiewicz wrote:
>> Hi Dmitry,
>>
>> W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
>>> Hi Andrzej,
>>>
>>> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>>>> Hi Dmitry,
>>>>
>>>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>>>> That said, I think the way we should handle inhibit/uninhibit, is that
>>>>> if we have the callback defined, then we call it, and only call open and
>>>>> close if uninhibit or inhibit are _not_ defined.
>>>>>
>>>>
>>>> If I understand you correctly you suggest to call either inhibit,
>>>> if provided or close, if inhibit is not provided, but not both,
>>>> that is, if both are provided then on the inhibit path only
>>>> inhibit is called. And, consequently, you suggest to call either
>>>> uninhibit or open, but not both. The rest of my mail makes this
>>>> assumption, so kindly confirm if I understand you correctly.
>>>
>>> Yes, that is correct. If a driver wants really fine-grained control, it
>>> will provide inhibit (or both inhibit and close), otherwise it will rely
>>> on close in place of inhibit.
>>>
>>>>
>>>> In my opinion this idea will not work.
>>>>
>>>> The first question is should we be able to inhibit a device
>>>> which is not opened? In my opinion we should, in order to be
>>>> able to inhibit a device in anticipation without needing to
>>>> open it first.
>>>
>>> I agree.
>>>
>>>>
>>>> Then what does opening (with input_open_device()) an inhibited
>>>> device mean? Should it succeed or should it fail?
>>>
>>> It should succeed.
>>>
>>>> If it is not
>>>> the first opening then effectively it boils down to increasing
>>>> device's and handle's counters, so we can allow it to succeed.
>>>> If, however, the device is being opened for the first time,
>>>> the ->open() method wants to be called, but that somehow
>>>> contradicts the device's inhibited state. So a logical thing
>>>> to do is to either fail input_open_device() or postpone ->open()
>>>> invocation to the moment of uninhibiting - and the latter is
>>>> what the patches in this series currently do.
>>>>
>>>> Failing input_open_device() because of the inhibited state is
>>>> not the right thing to do. Let me explain. Suppose that a device
>>>> is already inhibited and then a new matching handler appears
>>>> in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
>>>> mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
>>>> any character devices (only evdev.c, joydev.c and mousedev.c do),
>>>> so for them it makes no sense to delay calling input_open_device()
>>>> and it is called in handler's ->connect(). If input_open_device()
>>>> now fails, we have lost the only chance for this ->connect() to
>>>> succeed.
>>>>
>>>> Summarizing, IMO the uninhibit path should be calling both
>>>> ->open() and ->uninhibit() (if provided), and conversely, the inhibit
>>>> path should be calling both ->inhibit() and ->close() (if provided).
>>>
>>> So what you are trying to say is that you see inhibit as something that
>>> is done in addition to what happens in close. But what exactly do you
>>> want to do in inhibit, in addition to what close is doing?
>>
>> See below (*).
>>
>>>
>>> In my view, if we want to have a dedicated inhibit callback, then it
>>> will do everything that close does, they both are aware of each other
>>> and can sort out the state transitions between them. For drivers that do
>>> not have dedicated inhibit/uninhibit, we can use open and close
>>> handlers, and have input core sort out when each should be called. That
>>> means that we should not call dev->open() in input_open_device() when
>>> device is inhibited (and same for dev->close() in input_close_device).
>>> And when uninhibiting, we should not call dev->open() when there are no
>>> users for the device, and no dev->close() when inhibiting with no users.
>>>
>>> Do you see any problems with this approach?
>>
>> My concern is that if e.g. both ->open() and ->uninhibit() are provided,
>> then in certain circumstances ->open() won't be called:
>>
>> 1. users == 0
>> 2. inhibit happens
>> 3. input_open_device() happens, ->open() not called
>> 4. uninhibit happens
>> 5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.
>>
>> They way I understand your answer is that we implicitly impose requirements
>> on drivers which choose to implement e.g. both ->open() and ->uninhibit():
>> in such a case ->uninhibit() should be doing exactly the same things as
>> ->open() does. Which leads to a conclusion that in practice no drivers
>> should choose to implement both, otherwise they must be aware that
>> ->uninhibit() can be sometimes called instead of ->open(). Then ->open()
>> becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
>> Or, maybe, then ->inhibit() can be a superset of ->close() and
>> ->uninhibit() a superset of ->open().
>>
>> If such an approach is ok with you, it is ok with me, too.
>>
>> (*)
>> Calling both ->inhibit() and ->close() (if they are provided) allows
>> drivers to go fancy and fail inhibiting (which is impossible using
>> only ->close() as it does not return a value, but ->inhibit() by design
>> does). Then ->uninhibit() is mostly for symmetry.
>
> All the complications discussed above are exactly why I still
> believe that there should be only open and close.
>
> If error propagation on inhibit is considered as something
> really important to have then we can make the input driver close
> callback return an error (*), note I'm talking about the
> driver close callback here, not the system call.
>
> If the close callback is called for actually closing the fd
> referring to the input node, then the new error return code
> can be ignored, as we already do for errors on close atm
> since the driver close callback returns void.
>
> I still have not seen a very convincing argument for having
> separate inhibit and close callbacks and as the messy discussion
> above shows, having 2 such very similar yet subtly different
> calls seems like a bad idea...
>
> Regards,
>
> Hans
>
>
> *) This will require a flag day where "return 0" is added
> to all current close handlers
>
I'm taking one step back and looking at the ->open() and ->close()
driver callbacks. They are called from input_open_device() and
input_close_device(), respectively:
input_open_device():
"This function should be called by input handlers when they
want to start receive events from given input device."
->open() callback:
"this method is called when the very first user calls
input_open_device(). The driver must prepare the device to start
generating events (start polling thread, request an IRQ, submit
URB, etc.)"
input_close_device():
"This function should be called by input handlers when they
want to stop receive events from given input device."
->close() callback:
"this method is called when the very last user calls
input_close_device()"
It seems to me that the callback names do not reflect their
purpose: their meaning is not to "open" or to "close" but to
give drivers a chance to control when they start or stop
providing events to the input core.
What would you say about changing the callbacks' names?
I'd envsion: ->provide_events() instead of ->open() and
->stop_events() instead of ->close(). Of course drivers can
exploit the fact of knowing that nobody wants any events
from them and do whatever they consider appropriate, for
example go into a low power mode - but the latter is beyond
the scope of the input subsystem and is driver-specific.
With such a naming change in mind let's consider inhibiting.
We want to be able to control when to disregard events from
a given device. It makes sense to do it at device level, otherwise
such an operation would have to be invoked in all associated
handlers (those that have an open handle associating them with
the device in question). But of course we can do better than
merely ignoring the events received: we can tell the drivers
that we don't want any events from them, and later, at uninhibit
time, tell them to start providing the events again. Conceptually,
the two operations (provide or don't provide envents) are exactly
the same thing we want to be happening at input_open_device() and
input_close_device() time. To me, changing the names of
->open() and ->close() exposes this fact very well.
Consequently, ->inhibit() and ->uninhibit() won't be needed,
and drivers which already implement ->provide_events() (formerly
->open()) and ->stop_events() (formerly ->close()) will receive
full inhibit/uninhibit support for free (subject to how well they
implement ->provide_events()/->stop_events()). Unless we can come
up with what the drivers might be doing on top of ->stop_events()
and ->provide_events() when inhibiting/uninhibiting, but it seems
to me we can't. Can we?
Optionally ->close() (only the callback, not input_close_device())
can be made return a value, just as Hans suggests. The value
can be ignored in input_close_device() but used in input_inhibit().
No strong opinion here, though. (btw it seems to me that
input_inhibit() should be renamed to input_inhibit_device()).
Regards,
Andrzej
^ permalink raw reply
* [PATCH 2/2] clk: tegra: Always program PLL_E when enabled
From: Thierry Reding @ 2020-06-03 11:19 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Stephen Boyd, Jon Hunter, LABBE Corentin,
Dmitry Osipenko, linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200603111923.3545261-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Commit bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
added checks to avoid enabling PLLs that have already been enabled by
the bootloader. However, the PLL_E configuration inherited from the
bootloader isn't necessarily the one that is needed for the kernel.
This can cause SATA to fail like this:
[ 5.310270] phy phy-sata.6: phy poweron failed --> -110
[ 5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: -110
[ 5.323022] tegra-ahci: probe of 70027000.sata failed with error -110
Fix this by always programming the PLL_E. This ensures that any mis-
configuration by the bootloader will be overwritten by the kernel.
Fixes: bff1cef5f23a ("clk: tegra: Don't enable already enabled PLLs")
Reported-by: LABBE Corentin <clabbe-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
drivers/clk/tegra/clk-pll.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 583d2ac61e9e..b2d39a66f0fa 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -1601,9 +1601,6 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
unsigned long flags = 0;
unsigned long input_rate;
- if (clk_pll_is_enabled(hw))
- return 0;
-
input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
if (_get_table_rate(hw, &sel, pll->params->fixed_rate, input_rate))
--
2.24.1
^ permalink raw reply related
* [PATCH 1/2] clk: tegra: Capitalization fixes
From: Thierry Reding @ 2020-06-03 11:19 UTC (permalink / raw)
To: Thierry Reding
Cc: Michael Turquette, Stephen Boyd, Jon Hunter, LABBE Corentin,
Dmitry Osipenko, linux-clk, linux-tegra
From: Thierry Reding <treding@nvidia.com>
HW, XUSB and PLL are abbreviations and should be all-uppercase.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
drivers/clk/tegra/clk-pll.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 0b212cf2e794..583d2ac61e9e 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -1663,7 +1663,7 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
pll_writel(val, PLLE_SS_CTRL, pll);
udelay(1);
- /* Enable hw control of xusb brick pll */
+ /* Enable HW control of XUSB brick PLL */
val = pll_readl_misc(pll);
val &= ~PLLE_MISC_IDDQ_SW_CTRL;
pll_writel_misc(val, pll);
@@ -1686,7 +1686,7 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
val |= XUSBIO_PLL_CFG0_SEQ_ENABLE;
pll_writel(val, XUSBIO_PLL_CFG0, pll);
- /* Enable hw control of SATA pll */
+ /* Enable HW control of SATA PLL */
val = pll_readl(SATA_PLL_CFG0, pll);
val &= ~SATA_PLL_CFG0_PADPLL_RESET_SWCTL;
val |= SATA_PLL_CFG0_PADPLL_USE_LOCKDET;
--
2.24.1
^ permalink raw reply related
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Will Deacon @ 2020-06-03 11:11 UTC (permalink / raw)
To: Bjorn Andersson
Cc: John Stultz, Thierry Reding, Robin Murphy,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-arm-msm, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel
In-Reply-To: <20200602063210.GT11847@yoga>
On Mon, Jun 01, 2020 at 11:32:10PM -0700, Bjorn Andersson wrote:
> On Wed 27 May 04:03 PDT 2020, Will Deacon wrote:
>
> > Hi John, Bjorn,
> >
> > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > >
> > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > >
> > > > Rob, Will, we're reaching the point where upstream has enough
> > > > functionality that this is becoming a critical issue for us.
> > > >
> > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > similar on several devboards.
> > > >
> > > > As previously described, the only thing I want is the stream mapping
> > > > related to the display controller in place, either with the CB with
> > > > translation disabled or possibly with a way to specify the framebuffer
> > > > region (although this turns out to mess things up in the display
> > > > driver...)
> > > >
> > > > I did pick this up again recently and concluded that by omitting the
> > > > streams for the USB controllers causes an instability issue seen on one
> > > > of the controller to disappear. So I would prefer if we somehow could
> > > > have a mechanism to only pick the display streams and the context
> > > > allocation for this.
> > > >
> > > >
> > > > Can you please share some pointers/insights/wishes for how we can
> > > > conclude on this subject?
> > >
> > > Ping? I just wanted to follow up on this discussion as this small
> > > series is crucial for booting mainline on the Dragonboard 845c
> > > devboard. It would be really valuable to be able to get some solution
> > > upstream so we can test mainline w/o adding additional patches.
> >
> > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > about this on top of everything else. We're also carrying a hack in Android
> > for you :)
> >
>
> Thanks for taking the time to get back to us on this!
>
> > > The rest of the db845c series has been moving forward smoothly, but
> > > this set seems to be very stuck with no visible progress since Dec.
> > >
> > > Are there any pointers for what folks would prefer to see?
> >
> > I've had a chat with Robin about this. Originally, I was hoping that
> > people would all work together towards an idyllic future where firmware
> > would be able to describe arbitrary pre-existing mappings for devices,
> > irrespective of the IOMMU through which they master and Linux could
> > inherit this configuration. However, that hasn't materialised (there was
> > supposed to be an IORT update, but I don't know what happened to that)
> > and, in actual fact, the problem that you have on db845 is /far/ more
> > restricted than the general problem.
> >
> > Could you please try hacking something along the following lines and see
> > how you get on? You may need my for-joerg/arm-smmu/updates branch for
> > all the pieces:
> >
> > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
> > "pinning" and configure for bypass.
> >
> > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
> > for the display controller
> >
> > I /think/ that's sufficient, but note that it differs from the current
> > approach because we don't end up reserving a CB -- bypass is configured
> > in the S2CR instead. Some invalidation might therefore be needed in
> > ->cfg_probe() after unhooking the CB.
> >
> > Thanks, and please yell if you run into problems with this approach.
> >
>
> This sounded straight forward and cleaner, so I implemented it...
>
> Unfortunately the hypervisor is playing tricks on me when writing to
> S2CR registers:
> - TRANS writes lands as requested
> - BYPASS writes ends up in the register as requested, with type FAULT
> - FAULT writes are ignored
>
> In other words, the Qualcomm firmware prevents us from relying on
> marking the relevant streams as BYPASS type.
Is this for all S2CR registers, or only the ones in use by the display
controller? Is there any scope for stopping the hypervisor from doing this?
It makes it really difficult for the driver when the hardware is emulated
in a way that doesn't match the architecture...
Will
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Robin Murphy @ 2020-06-03 11:00 UTC (permalink / raw)
To: Bjorn Andersson, Will Deacon
Cc: linux-arm-msm, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Thierry Reding, John Stultz, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel
In-Reply-To: <20200602063210.GT11847@yoga>
On 2020-06-02 07:32, Bjorn Andersson wrote:
> On Wed 27 May 04:03 PDT 2020, Will Deacon wrote:
>
>> Hi John, Bjorn,
>>
>> On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
>>> On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>
>>>> On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
>>>>
>>>> Rob, Will, we're reaching the point where upstream has enough
>>>> functionality that this is becoming a critical issue for us.
>>>>
>>>> E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
>>>> mainline with display, GPU, WiFi and audio working and the story is
>>>> similar on several devboards.
>>>>
>>>> As previously described, the only thing I want is the stream mapping
>>>> related to the display controller in place, either with the CB with
>>>> translation disabled or possibly with a way to specify the framebuffer
>>>> region (although this turns out to mess things up in the display
>>>> driver...)
>>>>
>>>> I did pick this up again recently and concluded that by omitting the
>>>> streams for the USB controllers causes an instability issue seen on one
>>>> of the controller to disappear. So I would prefer if we somehow could
>>>> have a mechanism to only pick the display streams and the context
>>>> allocation for this.
>>>>
>>>>
>>>> Can you please share some pointers/insights/wishes for how we can
>>>> conclude on this subject?
>>>
>>> Ping? I just wanted to follow up on this discussion as this small
>>> series is crucial for booting mainline on the Dragonboard 845c
>>> devboard. It would be really valuable to be able to get some solution
>>> upstream so we can test mainline w/o adding additional patches.
>>
>> Sorry, it's been insanely busy recently and I haven't had a chance to think
>> about this on top of everything else. We're also carrying a hack in Android
>> for you :)
>>
>
> Thanks for taking the time to get back to us on this!
>
>>> The rest of the db845c series has been moving forward smoothly, but
>>> this set seems to be very stuck with no visible progress since Dec.
>>>
>>> Are there any pointers for what folks would prefer to see?
>>
>> I've had a chat with Robin about this. Originally, I was hoping that
>> people would all work together towards an idyllic future where firmware
>> would be able to describe arbitrary pre-existing mappings for devices,
>> irrespective of the IOMMU through which they master and Linux could
>> inherit this configuration. However, that hasn't materialised (there was
>> supposed to be an IORT update, but I don't know what happened to that)
>> and, in actual fact, the problem that you have on db845 is /far/ more
>> restricted than the general problem.
>>
>> Could you please try hacking something along the following lines and see
>> how you get on? You may need my for-joerg/arm-smmu/updates branch for
>> all the pieces:
>>
>> 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
>> "pinning" and configure for bypass.
>>
>> 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
>> for the display controller
>>
>> I /think/ that's sufficient, but note that it differs from the current
>> approach because we don't end up reserving a CB -- bypass is configured
>> in the S2CR instead. Some invalidation might therefore be needed in
>> ->cfg_probe() after unhooking the CB.
>>
>> Thanks, and please yell if you run into problems with this approach.
>>
>
> This sounded straight forward and cleaner, so I implemented it...
>
> Unfortunately the hypervisor is playing tricks on me when writing to
> S2CR registers:
> - TRANS writes lands as requested
> - BYPASS writes ends up in the register as requested, with type FAULT
> - FAULT writes are ignored
>
> In other words, the Qualcomm firmware prevents us from relying on
> marking the relevant streams as BYPASS type.
Sigh... at that point I'm inclined to suggest we give up and stop trying
to drive these things with arm-smmu. The XZR thing was bad enough, but
if they're not even going to pretend to implement the architecture
correctly then I'm not massively keen to continue tying the
architectural driver in further knots if innocent things like
CONFIG_IOMMU_DEFAULT_PASSTHROUGH are going to unexpectedly and
catastrophically fail. We have qcom-iommu for hypervisor-mediated SMMUs,
and this new hypervisor behaviour sounds to me more like "qcom-iommu++"
with reassignable stream-to-context mappings, rather than a proper Arm
SMMU emulation.
> Instead Qualcomm seems to implement "bypass" by setting up stream
> mapping, of TRANS type, pointing to a context bank without
> ARM_SMMU_SCTLR_M set.
...which arm-smmu specifically does not do because it's a silly waste of
resources - typically context banks are even scarcer than S2CRs.
Robin.
^ permalink raw reply
* Re: tegra124-jetson-tk1: sata doesnt work since 5.2
From: Thierry Reding @ 2020-06-03 10:56 UTC (permalink / raw)
To: LABBE Corentin, Peter De Schrijver
Cc: Dmitry Osipenko, jonathanh, pgaikwad, mturquette, sboyd, axboe,
linux-tegra, linux-kernel, linux-clk, linux-ide
In-Reply-To: <20200602081958.GA21773@Red>
[-- Attachment #1: Type: text/plain, Size: 7987 bytes --]
On Tue, Jun 02, 2020 at 10:19:58AM +0200, LABBE Corentin wrote:
> On Mon, Jun 01, 2020 at 05:30:37PM +0300, Dmitry Osipenko wrote:
> > 31.05.2020 22:31, LABBE Corentin пишет:
> > > On Thu, Mar 19, 2020 at 08:44:01AM +0100, LABBE Corentin wrote:
> > >> Hello
> > >>
> > >> sata doesnt work on tegra124-jetson-tk1 on next and master and at least since 5.2 (but 5.1 works).
> > >> [ 0.492810] +5V_SATA: supplied by +5V_SYS
> > >> [ 0.493230] +12V_SATA: supplied by +VDD_MUX
> > >> [ 2.088675] tegra-ahci 70027000.sata: 70027000.sata supply ahci not found, using dummy regulator
> > >> [ 2.097643] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, using dummy regulator
> > >> [ 3.314776] tegra-ahci 70027000.sata: 70027000.sata supply ahci not found, using dummy regulator
> > >> [ 3.323658] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, using dummy regulator
> > >> [ 5.236964] tegra-ahci 70027000.sata: 70027000.sata supply ahci not found, using dummy regulator
> > >> [ 5.245867] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, using dummy regulator
> > >> [ 5.254706] tegra-ahci 70027000.sata: 70027000.sata supply target not found, using dummy regulator
> > >> [ 5.310270] phy phy-sata.6: phy poweron failed --> -110
> > >> [ 5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: -110
> > >> [ 5.323022] tegra-ahci: probe of 70027000.sata failed with error -110
> > >> [ 35.694269] +5V_SATA: disabling
> > >> [ 35.697438] +12V_SATA: disabling
> > >>
> > >> I have bisected this problem:
> > >> git bisect start
> > >> # bad: [22c58fd70ca48a29505922b1563826593b08cc00] Merge tag 'armsoc-soc' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
> > >> git bisect bad 22c58fd70ca48a29505922b1563826593b08cc00
> > >> # good: [67e38f578aaebf34fc1278bbe45a78ee8c73dd33] ARM: ep93xx: move pinctrl interfaces into include/linux/soc
> > >> git bisect good 67e38f578aaebf34fc1278bbe45a78ee8c73dd33
> > >> # good: [80f232121b69cc69a31ccb2b38c1665d770b0710] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
> > >> git bisect good 80f232121b69cc69a31ccb2b38c1665d770b0710
> > >> # good: [e57ccca1ba33e1d92cc3bbf8b6304a46948844b0] Merge tag 'sound-5.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
> > >> git bisect good e57ccca1ba33e1d92cc3bbf8b6304a46948844b0
> > >> # bad: [983dfa4b6ee556563f7963348e4e2f97fc8a15b8] Merge tag 'for-linus-5.2-rc1' of ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/rw/uml
> > >> git bisect bad 983dfa4b6ee556563f7963348e4e2f97fc8a15b8
> > >> # good: [8e4ff713ce313dcabbb60e6ede1ffc193e67631f] Merge tag 'rtc-5.2' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux
> > >> git bisect good 8e4ff713ce313dcabbb60e6ede1ffc193e67631f
> > >> # bad: [b970afcfcabd63cd3832e95db096439c177c3592] Merge tag 'powerpc-5.2-1' of ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
> > >> git bisect bad b970afcfcabd63cd3832e95db096439c177c3592
> > >> # bad: [601e6bcc4ef02bda2831d5ac8133947b5edf597b] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> > >> git bisect bad 601e6bcc4ef02bda2831d5ac8133947b5edf597b
> > >> # good: [7e9c62bdb41af76974d594da89854a6aba645e58] Merge branches 'clk-sa', 'clk-aspeed', 'clk-samsung', 'clk-ingenic' and 'clk-zynq' into clk-next
> > >> git bisect good 7e9c62bdb41af76974d594da89854a6aba645e58
> > >> # bad: [0caf000817353cfc5db22363ecdac63b83d3a3f9] Merge branch 'clk-ti' into clk-next
> > >> git bisect bad 0caf000817353cfc5db22363ecdac63b83d3a3f9
> > >> # good: [5816b74581b45cf086a84ab14e13354a65e8e22c] Merge branches 'clk-hisi', 'clk-lochnagar', 'clk-allwinner', 'clk-rockchip' and 'clk-qoriq' into clk-next
> > >> git bisect good 5816b74581b45cf086a84ab14e13354a65e8e22c
> > >> # good: [7b4c162e03d47e037f8ee773c3e300eefb599a83] clk: at91: Mark struct clk_range as const
> > >> git bisect good 7b4c162e03d47e037f8ee773c3e300eefb599a83
> > >> # bad: [e71f4d385878671991e200083c7d30eb4ca8e99a] clk: tegra: divider: Mark Memory Controller clock as read-only
> > >> git bisect bad e71f4d385878671991e200083c7d30eb4ca8e99a
> > >> # bad: [924ee3d551c9deb16090230b824988bd37e72aa8] clk: tegra: emc: Don't enable EMC clock manually
> > >> git bisect bad 924ee3d551c9deb16090230b824988bd37e72aa8
> > >> # bad: [40db569d6769ffa3864fd1b89616b1a7323568a8] clk: tegra: Fix PLLM programming on Tegra124+ when PMC overrides divider
> > >> git bisect bad 40db569d6769ffa3864fd1b89616b1a7323568a8
> > >> # bad: [bff1cef5f23afbe49f5ebd766980dc612f5e9d0a] clk: tegra: Don't enable already enabled PLLs
> > >> git bisect bad bff1cef5f23afbe49f5ebd766980dc612f5e9d0a
> > >> # first bad commit: [bff1cef5f23afbe49f5ebd766980dc612f5e9d0a] clk: tegra: Don't enable already enabled PLLs
> > >>
> > >
> > > Hello
> > >
> > > I have digged a bit more and with the following "patch" I have now access to sata again
> > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > > index 0b212cf2e794..b4e2020051d5 100644
> > > --- a/drivers/clk/tegra/clk-pll.c
> > > +++ b/drivers/clk/tegra/clk-pll.c
> > > @@ -1602,7 +1603,7 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
> > > unsigned long input_rate;
> > >
> > > if (clk_pll_is_enabled(hw))
> > > - return 0;
> > > + pr_info("%s %s\n", __func__, clk_hw_get_name(&pll->hw));
> > >
> > > input_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
> > >
> > > This patch lead to a probed ahci, and I can see "clk_plle_tegra114_enable pll_e" in messages.
> > >
> > > So the bad part of bff1cef5f23afbe49f5ebd766980dc612f5e9d0a is found.
> >
> > Hello Labbe,
> >
> > Looks like indeed this PLLE change should be reverted. I see that the
> > code disables PLLE before changing its configuration, so apparently the
> > enable-check shouldn't be needed.
> >
> > > As additional information, my previous kernel has CONFIG_PHY_TEGRA_XUSB=m (since firmware is on rootfs which is on sata)
> > > but with this sata fail the same, having CONFIG_PHY_TEGRA_XUSB=y (and so xusb firmware in kernel) seems "necessary" for having SATA working.
> >
> > Sounds like PLLE needs to be enabled for SATA, but ahci-tegra driver
> > doesn't do that.
> >
> > Could you please try this change:
> >
> > --- >8 ---
> > diff --git a/drivers/clk/tegra/clk-tegra124.c
> > b/drivers/clk/tegra/clk-tegra124.c
> > index e931319dcc9d..7dbc14652830 100644
> > --- a/drivers/clk/tegra/clk-tegra124.c
> > +++ b/drivers/clk/tegra/clk-tegra124.c
> > @@ -1330,6 +1330,7 @@ static struct tegra_clk_init_table
> > common_init_table[] __initdata = {
> > { TEGRA124_CLK_I2S3_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
> > { TEGRA124_CLK_I2S4_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
> > { TEGRA124_CLK_VIMCLK_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
> > + { TEGRA124_CLK_PLL_E, TEGRA124_CLK_CLK_MAX, 0, 1 },
> > /* must be the last entry */
> > { TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0 },
> > };
> > --- >8 ---
>
> This patch alone does not fix the issue.
Looking at clk_plle_tegra114_enable(), there's a bunch of configuration
going on other than just enabling the PLL_E and it's likely that whoever
set up the PLL_E first (probably some bootloader) didn't have the same
requirements as the kernel has (most notably I can imagine that hardware
control of the XUSB and SATA PLLs remains disabled in the bootloader due
to the different way that they are used.
Peter, do you have any more background on what could be going on here or
who might be initializing the PLL_E (and others) in an incompatible way?
An alternative might be to identify who configures the PLL badly and fix
the culprit. But that's a bit of a strong dependency for my taste,
especially given that currenty versions of the bootloaders are broken.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4.9 00/55] 4.9.226-rc3 review
From: Jon Hunter @ 2020-06-03 10:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel
Cc: torvalds, akpm, linux, shuah, patches, ben.hutchings, lkft-triage,
stable, linux-tegra
In-Reply-To: <20200602181325.420361863@linuxfoundation.org>
On 02/06/2020 19:13, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.9.226 release.
> There are 55 patches in this series, all will be posted as a response
> to this one. If anyone has any issues with these being applied, please
> let me know.
>
> Responses should be made by Thu, 04 Jun 2020 18:12:28 +0000.
> Anything received after that time might be too late.
>
> The whole patch series can be found in one patch at:
> https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.226-rc3.gz
> or in the git tree and branch at:
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y
> and the diffstat can be found below.
>
> thanks,
>
> greg k-h
All tests are passing for Tegra. Seems that our test gremlins are still
at large and I cannot pull the report yet. However, I can see that
everything is passing fine.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Thierry Reding @ 2020-06-03 10:24 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-arm-msm, Robin Murphy,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, John Stultz,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Will Deacon, linux-arm-kernel
In-Reply-To: <20200602193249.GC1799770-Y7zrm0MriT3hXIiyNabO3w@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 7473 bytes --]
On Tue, Jun 02, 2020 at 12:32:49PM -0700, Bjorn Andersson wrote:
> On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote:
>
> > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote:
> > > Hi John, Bjorn,
> > >
> > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > > >
> > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > > >
> > > > > Rob, Will, we're reaching the point where upstream has enough
> > > > > functionality that this is becoming a critical issue for us.
> > > > >
> > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > > similar on several devboards.
> > > > >
> > > > > As previously described, the only thing I want is the stream mapping
> > > > > related to the display controller in place, either with the CB with
> > > > > translation disabled or possibly with a way to specify the framebuffer
> > > > > region (although this turns out to mess things up in the display
> > > > > driver...)
> > > > >
> > > > > I did pick this up again recently and concluded that by omitting the
> > > > > streams for the USB controllers causes an instability issue seen on one
> > > > > of the controller to disappear. So I would prefer if we somehow could
> > > > > have a mechanism to only pick the display streams and the context
> > > > > allocation for this.
> > > > >
> > > > >
> > > > > Can you please share some pointers/insights/wishes for how we can
> > > > > conclude on this subject?
> > > >
> > > > Ping? I just wanted to follow up on this discussion as this small
> > > > series is crucial for booting mainline on the Dragonboard 845c
> > > > devboard. It would be really valuable to be able to get some solution
> > > > upstream so we can test mainline w/o adding additional patches.
> > >
> > > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > > about this on top of everything else. We're also carrying a hack in Android
> > > for you :)
> > >
> > > > The rest of the db845c series has been moving forward smoothly, but
> > > > this set seems to be very stuck with no visible progress since Dec.
> > > >
> > > > Are there any pointers for what folks would prefer to see?
> > >
> > > I've had a chat with Robin about this. Originally, I was hoping that
> > > people would all work together towards an idyllic future where firmware
> > > would be able to describe arbitrary pre-existing mappings for devices,
> > > irrespective of the IOMMU through which they master and Linux could
> > > inherit this configuration. However, that hasn't materialised (there was
> > > supposed to be an IORT update, but I don't know what happened to that)
> > > and, in actual fact, the problem that you have on db845 is /far/ more
> > > restricted than the general problem.
> >
> > It doesn't sound to me like implementing platform-specific workarounds
> > is a good long-term solution (especially since, according to Bjorn, they
> > aren't as trivial to implement as it sounds). And we already have all
> > the infrastructure in place to implement what you describe, so I don't
> > see why we shouldn't do that. This patchset uses standard device tree
> > bindings that were designed for exactly this kind of use-case.
> >
>
> I think my results would imply that we would have to end up with (at
> least) some special case of your proposal (i.e. we need a context bank
> allocated).
I wasn't talking about implementation details, but rather about the
surrounding infrastructure. It seemed like Will was suggesting that
there's no way of conveying what memory regions to direct-map from
the firmware to the kernel. But that really isn't the problem here,
is it? What we're really looking for is how to take what we have in
device tree and use it in the ARM SMMU driver to create an early
mapping that will stay in place until a device has been properly
attached to the IOMMU domain.
> > So at least for device-tree based boot firmware can already describe
> > these pre-existing mappings. If something standard materializes for ACPI
> > eventually I'm sure we can find ways to integrate that into whatever we
> > come up with now for DT.
> >
> > I think between Bjorn, John, Laurentiu and myself there's pretty broad
> > consensus (correct me if I'm wrong, guys) that solving this via reserved
> > memory regions is a good solution that works. So I think what's really
> > missing is feedback on whether the changes proposed here or Laurentiu's
> > updated proposal[0] are acceptable, and if not, what the preference is
> > for getting something equivalent upstream.
> >
>
> As described in my reply to your proposal, the one problem I ran into
> was that I haven't figured out how to reliably "move" my display streams
> from one mapping entry to another.
>
> With the current scheme I see that their will either be gaps in time
> with no mapping for my display, or multiple mappings.
I think you would inevitably end up with two mappings for a transitional
period while you prepare the final mapping that you want to switch to.
> The other thing I noticed in your proposal was that I have a whole bunch
> of DT nodes with both iommus and memory-region properties that I really
> don't care to set up mappings for, but I've not finalized my thoughts on
> this causing actual problems...
Can you be more specific? It'd be useful to understand all of the
existing uses of reserved memory regions in order to make sure we
accomodate all of them.
I'd be surprised, though, if setting up mappings for any of these
regions would actually cause breakage. If a device tree node has a
memory-region property it means that this memory region is eventually
going to be used by the device and if the device tree node also has an
iommus property it means that it's meant to use the IOMMU for
translating memory accesses. It's therefore very likely that the memory
region will need to be mapped. Whether it needs to be a direct mapping
or not might be worth having a discussion about.
> > Just to highlight: the IOMMU framework already provides infrastructure
> > to create direct mappings (via iommu_get_resv_regions(), called from
> > iommu_create_device_direct_mappings()). I have patches that make use of
> > this on Tegra210 and earlier where a non-ARM SMMU is used and where the
> > IOMMU driver enables translations (and doesn't fault by default) only at
> > device attachment time. That works perfectly using reserved-memory
> > regions. Perhaps that infrastructure could be extended to cover the
> > kinds of early mappings that we're discussing here. On the other hand it
> > might be a bit premature at this point because the ARM SMMU is the only
> > device that currently needs this, as far as I can tell.
> >
>
> For Qualcomm we got patches picked up for 5.8 that will cause the
> display controller to be attached with direct mapping, so I think all
> missing now is the lack of stream mappings between arm-smmu probe and
> display driver probe...
Can you point me at those patches? I'd like to look at them and see if
they match what I have planned for Tegra or if we can somehow converge
on a common scheme.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] drm/atomic-helper: reset vblank on crtc reset
From: Daniel Vetter @ 2020-06-03 7:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: DRI Development, Intel Graphics Development, Boris Brezillon,
Liviu Dudau, Thierry Reding, Tetsuo Handa,
syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8,
James (Qian) Wang, Mihail Atanassov, Brian Starkey, Sam Ravnborg,
Boris Brezillon, Nicolas Ferre, Alexandre Belloni,
Ludovic Desroches, Maarten Lankhorst, Maxime Ripard <mrip>
In-Reply-To: <20200603001900.GV6547-N3hz7ZxfLydczECFQUw77jytWr6r+dGw0E9HWUfgJXw@public.gmane.org>
On Wed, Jun 3, 2020 at 2:19 AM Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> May I remind you about the -v option to git-format-patch ? :-) Seriously
> speaking, it really helps review.
>
> On Tue, Jun 02, 2020 at 11:51:38AM +0200, Daniel Vetter wrote:
> > Only when vblanks are supported ofc.
> >
> > Some drivers do this already, but most unfortunately missed it. This
> > opens up bugs after driver load, before the crtc is enabled for the
> > first time. syzbot spotted this when loading vkms as a secondary
> > output. Given how many drivers are buggy it's best to solve this once
> > and for all in shared helper code.
> >
> > Aside from moving the few existing calls to drm_crtc_vblank_reset into
> > helpers (i915 doesn't use helpers, so keeps its own) I think the
> > regression risk is minimal: atomic helpers already rely on drivers
> > calling drm_crtc_vblank_on/off correctly in their hooks when they
> > support vblanks. And driver that's failing to handle vblanks after
> > this is missing those calls already, and vblanks could only work by
> > accident when enabling a CRTC for the first time right after boot.
> >
> > Big thanks to Tetsuo for helping track down what's going wrong here.
> >
> > There's only a few drivers which already had the necessary call and
> > needed some updating:
> > - komeda, atmel and tidss also needed to be changed to call
> > __drm_atomic_helper_crtc_reset() intead of open coding it
> > - tegra and msm even had it in the same place already, just code
> > motion, and malidp already uses __drm_atomic_helper_crtc_reset().
> >
> > Only call left is in i915, which doesn't use drm_mode_config_reset,
> > but has its own fastboot infrastructure. So that's the only case where
> > we actually want this in the driver still.
> >
> > I've also reviewed all other drivers which set up vblank support with
> > drm_vblank_init. After the previous patch fixing mxsfb all atomic
> > drivers do call drm_crtc_vblank_on/off as they should, the remaining
> > drivers are either legacy kms or legacy dri1 drivers, so not affected
> > by this change to atomic helpers.
> >
> > v2: Use the drm_dev_has_vblank() helper.
> >
> > v3: Laurent pointed out that omap and rcar-du used drm_crtc_vblank_off
> > instead of drm_crtc_vblank_reset. Adjust them too.
> >
> > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > Reviewed-by: Boris Brezillon <boris.brezillon-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > Acked-by: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> > Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> > Reported-by: Tetsuo Handa <penguin-kernel-JPay3/Yim36HaxMnTkn67Xf5DAMn2ifp@public.gmane.org>
> > Reported-by: syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org
> > Cc: Tetsuo Handa <penguin-kernel-JPay3/Yim36HaxMnTkn67Xf5DAMn2ifp@public.gmane.org>
> > Cc: "James (Qian) Wang" <james.qian.wang-5wv7dgnIgG8@public.gmane.org>
> > Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mihail Atanassov <mihail.atanassov-5wv7dgnIgG8@public.gmane.org>
> > Cc: Brian Starkey <brian.starkey-5wv7dgnIgG8@public.gmane.org>
> > Cc: Sam Ravnborg <sam-uyr5N9Q2VtJg9hUCZPvPmw@public.gmane.org>
> > Cc: Boris Brezillon <bbrezillon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Nicolas Ferre <nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> > Cc: Alexandre Belloni <alexandre.belloni-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> > Cc: Ludovic Desroches <ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Cc: Maxime Ripard <mripard-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Thomas Zimmermann <tzimmermann-l3A5Bk7waGM@public.gmane.org>
> > Cc: David Airlie <airlied-cv59FeDIM0c@public.gmane.org>
> > Cc: Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>
> > Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Cc: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> > Cc: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> > Cc: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Cc: Brian Masney <masneyb-1iNe0GrtECGEi8DpZVb4nw@public.gmane.org>
> > Cc: Emil Velikov <emil.velikov-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > Cc: zhengbin <zhengbin13-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Signed-off-by: Daniel Vetter <daniel.vetter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-----
> > drivers/gpu/drm/arm/malidp_drv.c | 1 -
> > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-----
> > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 2 --
> > drivers/gpu/drm/omapdrm/omap_drv.c | 3 ---
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ---
> > drivers/gpu/drm/tegra/dc.c | 1 -
> > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> > drivers/gpu/drm/tidss/tidss_kms.c | 4 ----
> > 10 files changed, 9 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index 56bd938961ee..f33418d6e1a0 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
> > crtc->state = NULL;
> >
> > state = kzalloc(sizeof(*state), GFP_KERNEL);
> > - if (state) {
> > - crtc->state = &state->base;
> > - crtc->state->crtc = crtc;
> > - }
> > + if (state)
> > + __drm_atomic_helper_crtc_reset(crtc, &state->base);
> > }
> >
> > static struct drm_crtc_state *
> > @@ -616,7 +614,6 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
> > return err;
> >
> > drm_crtc_helper_add(crtc, &komeda_crtc_helper_funcs);
> > - drm_crtc_vblank_reset(crtc);
> >
> > crtc->port = kcrtc->master->of_output_port;
> >
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index c2507b7d8512..02904392e370 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -870,7 +870,6 @@ static int malidp_bind(struct device *dev)
> > drm->irq_enabled = true;
> >
> > ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> > - drm_crtc_vblank_reset(&malidp->crtc);
> > if (ret < 0) {
> > DRM_ERROR("failed to initialise vblank\n");
> > goto vblank_fail;
> > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> > index 10985134ce0b..ce246b96330b 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> > @@ -411,10 +411,8 @@ static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc)
> > }
> >
> > state = kzalloc(sizeof(*state), GFP_KERNEL);
> > - if (state) {
> > - crtc->state = &state->base;
> > - crtc->state->crtc = crtc;
> > - }
> > + if (state)
> > + __drm_atomic_helper_crtc_reset(crtc, &state->base);
> > }
> >
> > static struct drm_crtc_state *
> > @@ -528,7 +526,6 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev)
> > }
> >
> > drm_crtc_helper_add(&crtc->base, &lcdc_crtc_helper_funcs);
> > - drm_crtc_vblank_reset(&crtc->base);
> >
> > drm_mode_crtc_set_gamma_size(&crtc->base, ATMEL_HLCDC_CLUT_SIZE);
> > drm_crtc_enable_color_mgmt(&crtc->base, 0, false,
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 8fce6a115dfe..9ad74045158e 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -32,6 +32,7 @@
> > #include <drm/drm_device.h>
> > #include <drm/drm_plane.h>
> > #include <drm/drm_print.h>
> > +#include <drm/drm_vblank.h>
> > #include <drm/drm_writeback.h>
> >
> > #include <linux/slab.h>
> > @@ -93,6 +94,9 @@ __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
> > if (crtc_state)
> > __drm_atomic_helper_crtc_state_reset(crtc_state, crtc);
> >
> > + if (drm_dev_has_vblank(crtc->dev))
> > + drm_crtc_vblank_reset(crtc);
> > +
> > crtc->state = crtc_state;
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_crtc_reset);
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > index ca3368c828d0..9606185c284b 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > @@ -1117,8 +1117,6 @@ static void mdp5_crtc_reset(struct drm_crtc *crtc)
> > mdp5_crtc_destroy_state(crtc, crtc->state);
> >
> > __drm_atomic_helper_crtc_reset(crtc, &mdp5_cstate->base);
> > -
> > - drm_crtc_vblank_reset(crtc);
> > }
> >
> > static const struct drm_crtc_funcs mdp5_crtc_funcs = {
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > index 242d28281784..7a7066cded79 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > @@ -642,9 +642,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
> > goto err_cleanup_modeset;
> > }
> >
> > - for (i = 0; i < priv->num_pipes; i++)
> > - drm_crtc_vblank_off(priv->pipes[i].crtc);
> > -
> > omap_fbdev_init(ddev);
> >
> > drm_kms_helper_poll_init(ddev);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index d73e88ddecd0..e2959e32fd19 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -1271,9 +1271,6 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> >
> > drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> >
> > - /* Start with vertical blanking interrupt reporting disabled. */
> > - drm_crtc_vblank_off(crtc);
> > -
>
> I'm sure I've added this code for a good reason, I would have sworn that
> vblank would be either broken or would complain very loudly without this
> call. I've however tested this patch, and haven't noticed any
> regression. Has something changed since I've added the call to
> drm_crtc_vblank_off() in the rcar-du driver ? It can't be
> __drm_atomic_helper_crtc_reset() handling it, as the rcar-du driver has
> a custom CRTC reset handler that doesn't call
> __drm_atomic_helper_crtc_reset(). If it works without calling
> drm_crtc_vblank_off() or drm_crtc_vblank_reset(), what's the purpose of
> drm_crtc_vblank_reset() ?
>
> /me is very puzzled
Uh yeah this breaks stuff, I forgot to review the two drivers I've
changed to make sure they're using the __drm_atomic_helper_crtc_reset
function. Like they should. I'll respin.
-Daniel
> > /* Register the interrupt handler. */
> > if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
> > /* The IRQ's are associated with the CRTC (sw)index. */
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 83f31c6e891c..9b308b572eac 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -1168,7 +1168,6 @@ static void tegra_crtc_reset(struct drm_crtc *crtc)
> > tegra_crtc_atomic_destroy_state(crtc, crtc->state);
> >
> > __drm_atomic_helper_crtc_reset(crtc, &state->base);
> > - drm_crtc_vblank_reset(crtc);
> > }
> >
> > static struct drm_crtc_state *
> > diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> > index 89a226912de8..4d01c4af61cd 100644
> > --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> > +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> > @@ -352,8 +352,7 @@ static void tidss_crtc_reset(struct drm_crtc *crtc)
> > return;
> > }
> >
> > - crtc->state = &tcrtc->base;
> > - crtc->state->crtc = crtc;
> > + __drm_atomic_helper_crtc_reset(crtc, &tcrtc->base);
> > }
> >
> > static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
> > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> > index 4b99e9fa84a5..e6ab59eed259 100644
> > --- a/drivers/gpu/drm/tidss/tidss_kms.c
> > +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> > @@ -278,10 +278,6 @@ int tidss_modeset_init(struct tidss_device *tidss)
> > if (ret)
> > return ret;
> >
> > - /* Start with vertical blanking interrupt reporting disabled. */
> > - for (i = 0; i < tidss->num_crtcs; ++i)
> > - drm_crtc_vblank_reset(tidss->crtcs[i]);
> > -
> > drm_mode_config_reset(ddev);
> >
> > dev_dbg(tidss->dev, "%s done\n", __func__);
>
> --
> Regards,
>
> Laurent Pinchart
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH 4.14 00/76] 4.14.183-rc2 review
From: Greg Kroah-Hartman @ 2020-06-03 7:43 UTC (permalink / raw)
To: Jon Hunter
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
patches-ssFOTAMYnuFg9hUCZPvPmw,
ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
linux-tegra
In-Reply-To: <7e459417-74fe-c76b-58b9-66dccf12ab2b-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Tue, Jun 02, 2020 at 05:36:11PM +0100, Jon Hunter wrote:
>
> On 02/06/2020 11:23, Greg Kroah-Hartman wrote:
> > This is the start of the stable review cycle for the 4.14.183 release.
> > There are 76 patches in this series, all will be posted as a response
> > to this one. If anyone has any issues with these being applied, please
> > let me know.
> >
> > Responses should be made by Thu, 04 Jun 2020 10:16:52 +0000.
> > Anything received after that time might be too late.
> >
> > The whole patch series can be found in one patch at:
> > https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.183-rc2.gz
> > or in the git tree and branch at:
> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y
> > and the diffstat can be found below.
> >
> > thanks,
> >
> > greg k-h
>
>
> All tests are passing for Tegra ...
>
> Test results for stable-v4.14:
> 8 builds: 8 pass, 0 fail
> 16 boots: 16 pass, 0 fail
> 24 tests: 24 pass, 0 fail
>
> Linux version: 4.14.183-rc2-g3b54ad8f9c83
> Boards tested: tegra124-jetson-tk1, tegra20-ventana,
> tegra210-p2371-2180, tegra30-cardhu-a04
>
Great, thanks for testing and letting me know.
greg k-h
^ permalink raw reply
* Re: [PATCH 5.4 000/139] 5.4.44-rc2 review
From: Jon Hunter @ 2020-06-03 7:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
patches-ssFOTAMYnuFg9hUCZPvPmw,
ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
linux-tegra
In-Reply-To: <20200602101919.871080962-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
On 02/06/2020 11:24, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 5.4.44 release.
> There are 139 patches in this series, all will be posted as a response
> to this one. If anyone has any issues with these being applied, please
> let me know.
>
> Responses should be made by Thu, 04 Jun 2020 10:16:52 +0000.
> Anything received after that time might be too late.
>
> The whole patch series can be found in one patch at:
> https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.44-rc2.gz
> or in the git tree and branch at:
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y
> and the diffstat can be found below.
>
> thanks,
>
> greg k-h
All tests are passing for Tegra ...
Test results for stable-v5.4:
11 builds: 11 pass, 0 fail
26 boots: 26 pass, 0 fail
42 tests: 42 pass, 0 fail
Linux version: 5.4.44-rc2-g2ad6b0698b0e
Boards tested: tegra124-jetson-tk1, tegra186-p2771-0000,
tegra194-p2972-0000, tegra20-ventana,
tegra210-p2371-2180, tegra210-p3450-0000,
tegra30-cardhu-a04
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 4.19 00/92] 4.19.126-rc2 review
From: Jon Hunter @ 2020-06-03 7:29 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, shuah-DgEjT+Ai2ygdnm+yROfE0A,
patches-ssFOTAMYnuFg9hUCZPvPmw,
ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA,
lkft-triage-cunTk1MwBs8s++Sfvej+rw, stable-u79uwXL29TY76Z2rM5mHXA,
linux-tegra
In-Reply-To: <20200602101901.374486405-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
On 02/06/2020 11:23, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.19.126 release.
> There are 92 patches in this series, all will be posted as a response
> to this one. If anyone has any issues with these being applied, please
> let me know.
>
> Responses should be made by Thu, 04 Jun 2020 10:16:52 +0000.
> Anything received after that time might be too late.
>
> The whole patch series can be found in one patch at:
> https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.126-rc2.gz
> or in the git tree and branch at:
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y
> and the diffstat can be found below.
>
> thanks,
>
> greg k-h
All tests are passing for Tegra ...
Test results for stable-v4.19:
11 builds: 11 pass, 0 fail
22 boots: 22 pass, 0 fail
32 tests: 32 pass, 0 fail
Linux version: 4.19.126-rc2-gf6262a450952
Boards tested: tegra124-jetson-tk1, tegra186-p2771-0000,
tegra194-p2972-0000, tegra20-ventana,
tegra210-p2371-2180, tegra30-cardhu-a04
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 1/3] drm/atomic-helper: reset vblank on crtc reset
From: Laurent Pinchart @ 2020-06-03 0:19 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, Boris Brezillon,
Liviu Dudau, Thierry Reding, Tetsuo Handa,
syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8,
James (Qian) Wang, Mihail Atanassov, Brian Starkey, Sam Ravnborg,
Boris Brezillon, Nicolas Ferre, Alexandre Belloni,
Ludovic Desroches, Maarten Lankhorst, Maxime Ripard <mrip>
In-Reply-To: <20200602095140.36678-1-daniel.vetter-/w4YWyX8dFk@public.gmane.org>
Hi Daniel,
Thank you for the patch.
May I remind you about the -v option to git-format-patch ? :-) Seriously
speaking, it really helps review.
On Tue, Jun 02, 2020 at 11:51:38AM +0200, Daniel Vetter wrote:
> Only when vblanks are supported ofc.
>
> Some drivers do this already, but most unfortunately missed it. This
> opens up bugs after driver load, before the crtc is enabled for the
> first time. syzbot spotted this when loading vkms as a secondary
> output. Given how many drivers are buggy it's best to solve this once
> and for all in shared helper code.
>
> Aside from moving the few existing calls to drm_crtc_vblank_reset into
> helpers (i915 doesn't use helpers, so keeps its own) I think the
> regression risk is minimal: atomic helpers already rely on drivers
> calling drm_crtc_vblank_on/off correctly in their hooks when they
> support vblanks. And driver that's failing to handle vblanks after
> this is missing those calls already, and vblanks could only work by
> accident when enabling a CRTC for the first time right after boot.
>
> Big thanks to Tetsuo for helping track down what's going wrong here.
>
> There's only a few drivers which already had the necessary call and
> needed some updating:
> - komeda, atmel and tidss also needed to be changed to call
> __drm_atomic_helper_crtc_reset() intead of open coding it
> - tegra and msm even had it in the same place already, just code
> motion, and malidp already uses __drm_atomic_helper_crtc_reset().
>
> Only call left is in i915, which doesn't use drm_mode_config_reset,
> but has its own fastboot infrastructure. So that's the only case where
> we actually want this in the driver still.
>
> I've also reviewed all other drivers which set up vblank support with
> drm_vblank_init. After the previous patch fixing mxsfb all atomic
> drivers do call drm_crtc_vblank_on/off as they should, the remaining
> drivers are either legacy kms or legacy dri1 drivers, so not affected
> by this change to atomic helpers.
>
> v2: Use the drm_dev_has_vblank() helper.
>
> v3: Laurent pointed out that omap and rcar-du used drm_crtc_vblank_off
> instead of drm_crtc_vblank_reset. Adjust them too.
>
> Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Reviewed-by: Boris Brezillon <boris.brezillon-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Acked-by: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Link: https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb
> Reported-by: Tetsuo Handa <penguin-kernel-JPay3/Yim36HaxMnTkn67Xf5DAMn2ifp@public.gmane.org>
> Reported-by: syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org
> Cc: Tetsuo Handa <penguin-kernel-JPay3/Yim36HaxMnTkn67Xf5DAMn2ifp@public.gmane.org>
> Cc: "James (Qian) Wang" <james.qian.wang-5wv7dgnIgG8@public.gmane.org>
> Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> Cc: Mihail Atanassov <mihail.atanassov-5wv7dgnIgG8@public.gmane.org>
> Cc: Brian Starkey <brian.starkey-5wv7dgnIgG8@public.gmane.org>
> Cc: Sam Ravnborg <sam-uyr5N9Q2VtJg9hUCZPvPmw@public.gmane.org>
> Cc: Boris Brezillon <bbrezillon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> Cc: Alexandre Belloni <alexandre.belloni-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> Cc: Ludovic Desroches <ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> Cc: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Maxime Ripard <mripard-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Thomas Zimmermann <tzimmermann-l3A5Bk7waGM@public.gmane.org>
> Cc: David Airlie <airlied-cv59FeDIM0c@public.gmane.org>
> Cc: Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>
> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> Cc: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> Cc: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Brian Masney <masneyb-1iNe0GrtECGEi8DpZVb4nw@public.gmane.org>
> Cc: Emil Velikov <emil.velikov-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Cc: zhengbin <zhengbin13-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Daniel Vetter <daniel.vetter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++-----
> drivers/gpu/drm/arm/malidp_drv.c | 1 -
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++-----
> drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 2 --
> drivers/gpu/drm/omapdrm/omap_drv.c | 3 ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ---
> drivers/gpu/drm/tegra/dc.c | 1 -
> drivers/gpu/drm/tidss/tidss_crtc.c | 3 +--
> drivers/gpu/drm/tidss/tidss_kms.c | 4 ----
> 10 files changed, 9 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 56bd938961ee..f33418d6e1a0 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -492,10 +492,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
> crtc->state = NULL;
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = &state->base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, &state->base);
> }
>
> static struct drm_crtc_state *
> @@ -616,7 +614,6 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
> return err;
>
> drm_crtc_helper_add(crtc, &komeda_crtc_helper_funcs);
> - drm_crtc_vblank_reset(crtc);
>
> crtc->port = kcrtc->master->of_output_port;
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index c2507b7d8512..02904392e370 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -870,7 +870,6 @@ static int malidp_bind(struct device *dev)
> drm->irq_enabled = true;
>
> ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> - drm_crtc_vblank_reset(&malidp->crtc);
> if (ret < 0) {
> DRM_ERROR("failed to initialise vblank\n");
> goto vblank_fail;
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 10985134ce0b..ce246b96330b 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -411,10 +411,8 @@ static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc)
> }
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = &state->base;
> - crtc->state->crtc = crtc;
> - }
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, &state->base);
> }
>
> static struct drm_crtc_state *
> @@ -528,7 +526,6 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev)
> }
>
> drm_crtc_helper_add(&crtc->base, &lcdc_crtc_helper_funcs);
> - drm_crtc_vblank_reset(&crtc->base);
>
> drm_mode_crtc_set_gamma_size(&crtc->base, ATMEL_HLCDC_CLUT_SIZE);
> drm_crtc_enable_color_mgmt(&crtc->base, 0, false,
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 8fce6a115dfe..9ad74045158e 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -32,6 +32,7 @@
> #include <drm/drm_device.h>
> #include <drm/drm_plane.h>
> #include <drm/drm_print.h>
> +#include <drm/drm_vblank.h>
> #include <drm/drm_writeback.h>
>
> #include <linux/slab.h>
> @@ -93,6 +94,9 @@ __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
> if (crtc_state)
> __drm_atomic_helper_crtc_state_reset(crtc_state, crtc);
>
> + if (drm_dev_has_vblank(crtc->dev))
> + drm_crtc_vblank_reset(crtc);
> +
> crtc->state = crtc_state;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_crtc_reset);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index ca3368c828d0..9606185c284b 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -1117,8 +1117,6 @@ static void mdp5_crtc_reset(struct drm_crtc *crtc)
> mdp5_crtc_destroy_state(crtc, crtc->state);
>
> __drm_atomic_helper_crtc_reset(crtc, &mdp5_cstate->base);
> -
> - drm_crtc_vblank_reset(crtc);
> }
>
> static const struct drm_crtc_funcs mdp5_crtc_funcs = {
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 242d28281784..7a7066cded79 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -642,9 +642,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
> goto err_cleanup_modeset;
> }
>
> - for (i = 0; i < priv->num_pipes; i++)
> - drm_crtc_vblank_off(priv->pipes[i].crtc);
> -
> omap_fbdev_init(ddev);
>
> drm_kms_helper_poll_init(ddev);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index d73e88ddecd0..e2959e32fd19 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1271,9 +1271,6 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>
> drm_crtc_helper_add(crtc, &crtc_helper_funcs);
>
> - /* Start with vertical blanking interrupt reporting disabled. */
> - drm_crtc_vblank_off(crtc);
> -
I'm sure I've added this code for a good reason, I would have sworn that
vblank would be either broken or would complain very loudly without this
call. I've however tested this patch, and haven't noticed any
regression. Has something changed since I've added the call to
drm_crtc_vblank_off() in the rcar-du driver ? It can't be
__drm_atomic_helper_crtc_reset() handling it, as the rcar-du driver has
a custom CRTC reset handler that doesn't call
__drm_atomic_helper_crtc_reset(). If it works without calling
drm_crtc_vblank_off() or drm_crtc_vblank_reset(), what's the purpose of
drm_crtc_vblank_reset() ?
/me is very puzzled
> /* Register the interrupt handler. */
> if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
> /* The IRQ's are associated with the CRTC (sw)index. */
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 83f31c6e891c..9b308b572eac 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1168,7 +1168,6 @@ static void tegra_crtc_reset(struct drm_crtc *crtc)
> tegra_crtc_atomic_destroy_state(crtc, crtc->state);
>
> __drm_atomic_helper_crtc_reset(crtc, &state->base);
> - drm_crtc_vblank_reset(crtc);
> }
>
> static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 89a226912de8..4d01c4af61cd 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -352,8 +352,7 @@ static void tidss_crtc_reset(struct drm_crtc *crtc)
> return;
> }
>
> - crtc->state = &tcrtc->base;
> - crtc->state->crtc = crtc;
> + __drm_atomic_helper_crtc_reset(crtc, &tcrtc->base);
> }
>
> static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 4b99e9fa84a5..e6ab59eed259 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -278,10 +278,6 @@ int tidss_modeset_init(struct tidss_device *tidss)
> if (ret)
> return ret;
>
> - /* Start with vertical blanking interrupt reporting disabled. */
> - for (i = 0; i < tidss->num_crtcs; ++i)
> - drm_crtc_vblank_reset(tidss->crtcs[i]);
> -
> drm_mode_config_reset(ddev);
>
> dev_dbg(tidss->dev, "%s done\n", __func__);
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: tegra124-jetson-tk1: sata doesnt work since 5.2
From: Dmitry Osipenko @ 2020-06-02 22:49 UTC (permalink / raw)
To: LABBE Corentin
Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
jonathanh-DDmLM1+adcrQT0dZR+AlfA, pgaikwad-DDmLM1+adcrQT0dZR+AlfA,
pdeschrijver-DDmLM1+adcrQT0dZR+AlfA,
mturquette-rdvid1DuHRBWk0Htik3J/w, sboyd-DgEjT+Ai2ygdnm+yROfE0A,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-ide-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200602081958.GA21773@Red>
02.06.2020 11:19, LABBE Corentin пишет:
...
>> Sounds like PLLE needs to be enabled for SATA, but ahci-tegra driver
>> doesn't do that.
>>
>> Could you please try this change:
>>
>> --- >8 ---
>> diff --git a/drivers/clk/tegra/clk-tegra124.c
>> b/drivers/clk/tegra/clk-tegra124.c
>> index e931319dcc9d..7dbc14652830 100644
>> --- a/drivers/clk/tegra/clk-tegra124.c
>> +++ b/drivers/clk/tegra/clk-tegra124.c
>> @@ -1330,6 +1330,7 @@ static struct tegra_clk_init_table
>> common_init_table[] __initdata = {
>> { TEGRA124_CLK_I2S3_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
>> { TEGRA124_CLK_I2S4_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
>> { TEGRA124_CLK_VIMCLK_SYNC, TEGRA124_CLK_CLK_MAX, 24576000, 0 },
>> + { TEGRA124_CLK_PLL_E, TEGRA124_CLK_CLK_MAX, 0, 1 },
>> /* must be the last entry */
>> { TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0 },
>> };
>> --- >8 ---
>
> This patch alone does not fix the issue.
I'd try to enable all the clocks and voltage regulators that are enabled
by the USB driver. Also won't hurt to check the clock rate
configurations with the enabled / disabled USB, try to replicate the
working configuration.
^ permalink raw reply
* Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
From: Jerry Snitselaar @ 2020-06-02 21:17 UTC (permalink / raw)
To: Joerg Roedel, Lu Baolu, Will Deacon, Robin Murphy,
Marek Szyprowski, Kukjin Kim, Krzysztof Kozlowski,
David Woodhouse, Andy Gross, Bjorn Andersson, Matthias Brugger,
Rob Clark, Heiko Stuebner, Gerald Schaefer, Thierry Reding,
Jonathan Hunter, Jean-Philippe Brucker,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200602163806.o5dpj2tpemwdzyiw@cantor>
On Tue Jun 02 20, Jerry Snitselaar wrote:
>On Tue Jun 02 20, Joerg Roedel wrote:
>>Hi Jerry,
>>
>>On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
>>>
>>>Yeah, that will solve the panic.
>>>
>>
>>If you still see the kdump faults, can you please try with the attached
>>diff? I was not able to reproduce them in my setup.
>>
>>Regards,
>>
>> Joerg
>>
>
>I have another hp proliant server now, and reproduced. I will have the
>patch below tested shortly. Minor change, I switched group->domain to
>domain since group isn't an argument, and *data being passed in comes
>from group->domain anyways.
>
Looks like it solves problem for both the epyc system, and the hp proliant
server,
>>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>index b5ea203f6c68..5a6d509f72b6 100644
>>--- a/drivers/iommu/iommu.c
>>+++ b/drivers/iommu/iommu.c
>>@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>>static int iommu_group_do_dma_attach(struct device *dev, void *data)
>>{
>> struct iommu_domain *domain = data;
>>+ int ret = 0;
>>
>>- return __iommu_attach_device(domain, dev);
>>+ if (!iommu_is_attach_deferred(group->domain, dev))
>>+ ret = __iommu_attach_device(group->domain, dev);
>>+
>>+ return ret;
>>}
>>
>>static int __iommu_group_dma_attach(struct iommu_group *group)
>>_______________________________________________
>>iommu mailing list
>>iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
^ permalink raw reply
* Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
From: Jerry Snitselaar @ 2020-06-02 21:17 UTC (permalink / raw)
To: Joerg Roedel, Lu Baolu, Will Deacon, Robin Murphy,
Marek Szyprowski, Kukjin Kim, Krzysztof Kozlowski,
David Woodhouse, Andy Gross, Bjorn Andersson, Matthias Brugger,
Rob Clark, Heiko Stuebner, Gerald Schaefer, Thierry Reding,
Jonathan Hunter, Jean-Philippe Brucker,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
iommu-cunTk1MwBs+XwC/5MpYmKg
In-Reply-To: <20200602163806.o5dpj2tpemwdzyiw@cantor>
On Tue Jun 02 20, Jerry Snitselaar wrote:
>On Tue Jun 02 20, Joerg Roedel wrote:
>>Hi Jerry,
>>
>>On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
>>>
>>>Yeah, that will solve the panic.
>>>
>>
>>If you still see the kdump faults, can you please try with the attached
>>diff? I was not able to reproduce them in my setup.
>>
>>Regards,
>>
>> Joerg
>>
>
>I have another hp proliant server now, and reproduced. I will have the
>patch below tested shortly. Minor change, I switched group->domain to
>domain since group isn't an argument, and *data being passed in comes
>from group->domain anyways.
>
Looks like it solves problem for both the epyc system, and the hp proliant
server,
>>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>index b5ea203f6c68..5a6d509f72b6 100644
>>--- a/drivers/iommu/iommu.c
>>+++ b/drivers/iommu/iommu.c
>>@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>>static int iommu_group_do_dma_attach(struct device *dev, void *data)
>>{
>> struct iommu_domain *domain = data;
>>+ int ret = 0;
>>
>>- return __iommu_attach_device(domain, dev);
>>+ if (!iommu_is_attach_deferred(group->domain, dev))
>>+ ret = __iommu_attach_device(group->domain, dev);
>>+
>>+ return ret;
>>}
>>
>>static int __iommu_group_dma_attach(struct iommu_group *group)
>>_______________________________________________
>>iommu mailing list
>>iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
^ permalink raw reply
* Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
From: Jerry Snitselaar @ 2020-06-02 21:17 UTC (permalink / raw)
To: Joerg Roedel, Lu Baolu, Will Deacon, Robin Murphy,
Marek Szyprowski, Kukjin Kim, Krzysztof Kozlowski,
David Woodhouse, Andy Gross, Bjorn Andersson, Matthias Brugger,
Rob Clark, Heiko Stuebner, Gerald Schaefer, Thierry Reding,
Jonathan Hunter, Jean-Philippe Brucker,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
iommu-cunTk1MwBs+XwC/5MpYmKg
In-Reply-To: <20200602163806.o5dpj2tpemwdzyiw@cantor>
On Tue Jun 02 20, Jerry Snitselaar wrote:
>On Tue Jun 02 20, Joerg Roedel wrote:
>>Hi Jerry,
>>
>>On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
>>>
>>>Yeah, that will solve the panic.
>>>
>>
>>If you still see the kdump faults, can you please try with the attached
>>diff? I was not able to reproduce them in my setup.
>>
>>Regards,
>>
>> Joerg
>>
>
>I have another hp proliant server now, and reproduced. I will have the
>patch below tested shortly. Minor change, I switched group->domain to
>domain since group isn't an argument, and *data being passed in comes
>from group->domain anyways.
>
Looks like it solves problem for both the epyc system, and the hp proliant
server,
>>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>index b5ea203f6c68..5a6d509f72b6 100644
>>--- a/drivers/iommu/iommu.c
>>+++ b/drivers/iommu/iommu.c
>>@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>>static int iommu_group_do_dma_attach(struct device *dev, void *data)
>>{
>> struct iommu_domain *domain = data;
>>+ int ret = 0;
>>
>>- return __iommu_attach_device(domain, dev);
>>+ if (!iommu_is_attach_deferred(group->domain, dev))
>>+ ret = __iommu_attach_device(group->domain, dev);
>>+
>>+ return ret;
>>}
>>
>>static int __iommu_group_dma_attach(struct iommu_group *group)
>>_______________________________________________
>>iommu mailing list
>>iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
^ permalink raw reply
* Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
From: Jerry Snitselaar @ 2020-06-02 21:17 UTC (permalink / raw)
To: Joerg Roedel, Lu Baolu, Will Deacon, Robin Murphy,
Marek Szyprowski, Kukjin Kim, Krzysztof Kozlowski,
David Woodhouse, Andy Gross, Bjorn Andersson, Matthias Brugger,
Rob Clark, Heiko Stuebner, Gerald Schaefer, Thierry Reding,
Jonathan Hunter, Jean-Philippe Brucker,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
iommu-cunTk1MwBs+XwC/5MpYmKg
In-Reply-To: <20200602163806.o5dpj2tpemwdzyiw@cantor>
On Tue Jun 02 20, Jerry Snitselaar wrote:
>On Tue Jun 02 20, Joerg Roedel wrote:
>>Hi Jerry,
>>
>>On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
>>>
>>>Yeah, that will solve the panic.
>>>
>>
>>If you still see the kdump faults, can you please try with the attached
>>diff? I was not able to reproduce them in my setup.
>>
>>Regards,
>>
>> Joerg
>>
>
>I have another hp proliant server now, and reproduced. I will have the
>patch below tested shortly. Minor change, I switched group->domain to
>domain since group isn't an argument, and *data being passed in comes
>from group->domain anyways.
>
Looks like it solves problem for both the epyc system, and the hp proliant
server,
>>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>index b5ea203f6c68..5a6d509f72b6 100644
>>--- a/drivers/iommu/iommu.c
>>+++ b/drivers/iommu/iommu.c
>>@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>>static int iommu_group_do_dma_attach(struct device *dev, void *data)
>>{
>> struct iommu_domain *domain = data;
>>+ int ret = 0;
>>
>>- return __iommu_attach_device(domain, dev);
>>+ if (!iommu_is_attach_deferred(group->domain, dev))
>>+ ret = __iommu_attach_device(group->domain, dev);
>>+
>>+ return ret;
>>}
>>
>>static int __iommu_group_dma_attach(struct iommu_group *group)
>>_______________________________________________
>>iommu mailing list
>>iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Hans de Goede @ 2020-06-02 20:19 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, Dmitry Torokhov
Cc: linux-input, linux-acpi, linux-iio, linux-arm-kernel,
linux-samsung-soc, linux-tegra, patches, ibm-acpi-devel,
platform-driver-x86, Rafael J . Wysocki, Len Brown,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <82e9f2ab-a16e-51ee-1413-bedf0122026a@collabora.com>
Hi,
On 6/2/20 8:50 PM, Andrzej Pietrasiewicz wrote:
> Hi Dmitry,
>
> W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
>> Hi Andrzej,
>>
>> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>>> Hi Dmitry,
>>>
>>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>>> That said, I think the way we should handle inhibit/uninhibit, is that
>>>> if we have the callback defined, then we call it, and only call open and
>>>> close if uninhibit or inhibit are _not_ defined.
>>>>
>>>
>>> If I understand you correctly you suggest to call either inhibit,
>>> if provided or close, if inhibit is not provided, but not both,
>>> that is, if both are provided then on the inhibit path only
>>> inhibit is called. And, consequently, you suggest to call either
>>> uninhibit or open, but not both. The rest of my mail makes this
>>> assumption, so kindly confirm if I understand you correctly.
>>
>> Yes, that is correct. If a driver wants really fine-grained control, it
>> will provide inhibit (or both inhibit and close), otherwise it will rely
>> on close in place of inhibit.
>>
>>>
>>> In my opinion this idea will not work.
>>>
>>> The first question is should we be able to inhibit a device
>>> which is not opened? In my opinion we should, in order to be
>>> able to inhibit a device in anticipation without needing to
>>> open it first.
>>
>> I agree.
>>
>>>
>>> Then what does opening (with input_open_device()) an inhibited
>>> device mean? Should it succeed or should it fail?
>>
>> It should succeed.
>>
>>> If it is not
>>> the first opening then effectively it boils down to increasing
>>> device's and handle's counters, so we can allow it to succeed.
>>> If, however, the device is being opened for the first time,
>>> the ->open() method wants to be called, but that somehow
>>> contradicts the device's inhibited state. So a logical thing
>>> to do is to either fail input_open_device() or postpone ->open()
>>> invocation to the moment of uninhibiting - and the latter is
>>> what the patches in this series currently do.
>>>
>>> Failing input_open_device() because of the inhibited state is
>>> not the right thing to do. Let me explain. Suppose that a device
>>> is already inhibited and then a new matching handler appears
>>> in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
>>> mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
>>> any character devices (only evdev.c, joydev.c and mousedev.c do),
>>> so for them it makes no sense to delay calling input_open_device()
>>> and it is called in handler's ->connect(). If input_open_device()
>>> now fails, we have lost the only chance for this ->connect() to
>>> succeed.
>>>
>>> Summarizing, IMO the uninhibit path should be calling both
>>> ->open() and ->uninhibit() (if provided), and conversely, the inhibit
>>> path should be calling both ->inhibit() and ->close() (if provided).
>>
>> So what you are trying to say is that you see inhibit as something that
>> is done in addition to what happens in close. But what exactly do you
>> want to do in inhibit, in addition to what close is doing?
>
> See below (*).
>
>>
>> In my view, if we want to have a dedicated inhibit callback, then it
>> will do everything that close does, they both are aware of each other
>> and can sort out the state transitions between them. For drivers that do
>> not have dedicated inhibit/uninhibit, we can use open and close
>> handlers, and have input core sort out when each should be called. That
>> means that we should not call dev->open() in input_open_device() when
>> device is inhibited (and same for dev->close() in input_close_device).
>> And when uninhibiting, we should not call dev->open() when there are no
>> users for the device, and no dev->close() when inhibiting with no users.
>>
>> Do you see any problems with this approach?
>
> My concern is that if e.g. both ->open() and ->uninhibit() are provided,
> then in certain circumstances ->open() won't be called:
>
> 1. users == 0
> 2. inhibit happens
> 3. input_open_device() happens, ->open() not called
> 4. uninhibit happens
> 5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.
>
> They way I understand your answer is that we implicitly impose requirements
> on drivers which choose to implement e.g. both ->open() and ->uninhibit():
> in such a case ->uninhibit() should be doing exactly the same things as
> ->open() does. Which leads to a conclusion that in practice no drivers
> should choose to implement both, otherwise they must be aware that
> ->uninhibit() can be sometimes called instead of ->open(). Then ->open()
> becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
> Or, maybe, then ->inhibit() can be a superset of ->close() and
> ->uninhibit() a superset of ->open().
>
> If such an approach is ok with you, it is ok with me, too.
>
> (*)
> Calling both ->inhibit() and ->close() (if they are provided) allows
> drivers to go fancy and fail inhibiting (which is impossible using
> only ->close() as it does not return a value, but ->inhibit() by design
> does). Then ->uninhibit() is mostly for symmetry.
All the complications discussed above are exactly why I still
believe that there should be only open and close.
If error propagation on inhibit is considered as something
really important to have then we can make the input driver close
callback return an error (*), note I'm talking about the
driver close callback here, not the system call.
If the close callback is called for actually closing the fd
referring to the input node, then the new error return code
can be ignored, as we already do for errors on close atm
since the driver close callback returns void.
I still have not seen a very convincing argument for having
separate inhibit and close callbacks and as the messy discussion
above shows, having 2 such very similar yet subtly different
calls seems like a bad idea...
Regards,
Hans
*) This will require a flag day where "return 0" is added
to all current close handlers
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Bjorn Andersson @ 2020-06-02 19:32 UTC (permalink / raw)
To: Thierry Reding
Cc: Will Deacon, John Stultz, Robin Murphy, Laurentiu Tudor,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-arm-msm, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-arm-kernel
In-Reply-To: <20200602110216.GA3354422@ulmo>
On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote:
> On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote:
> > Hi John, Bjorn,
> >
> > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > >
> > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > >
> > > > Rob, Will, we're reaching the point where upstream has enough
> > > > functionality that this is becoming a critical issue for us.
> > > >
> > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > similar on several devboards.
> > > >
> > > > As previously described, the only thing I want is the stream mapping
> > > > related to the display controller in place, either with the CB with
> > > > translation disabled or possibly with a way to specify the framebuffer
> > > > region (although this turns out to mess things up in the display
> > > > driver...)
> > > >
> > > > I did pick this up again recently and concluded that by omitting the
> > > > streams for the USB controllers causes an instability issue seen on one
> > > > of the controller to disappear. So I would prefer if we somehow could
> > > > have a mechanism to only pick the display streams and the context
> > > > allocation for this.
> > > >
> > > >
> > > > Can you please share some pointers/insights/wishes for how we can
> > > > conclude on this subject?
> > >
> > > Ping? I just wanted to follow up on this discussion as this small
> > > series is crucial for booting mainline on the Dragonboard 845c
> > > devboard. It would be really valuable to be able to get some solution
> > > upstream so we can test mainline w/o adding additional patches.
> >
> > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > about this on top of everything else. We're also carrying a hack in Android
> > for you :)
> >
> > > The rest of the db845c series has been moving forward smoothly, but
> > > this set seems to be very stuck with no visible progress since Dec.
> > >
> > > Are there any pointers for what folks would prefer to see?
> >
> > I've had a chat with Robin about this. Originally, I was hoping that
> > people would all work together towards an idyllic future where firmware
> > would be able to describe arbitrary pre-existing mappings for devices,
> > irrespective of the IOMMU through which they master and Linux could
> > inherit this configuration. However, that hasn't materialised (there was
> > supposed to be an IORT update, but I don't know what happened to that)
> > and, in actual fact, the problem that you have on db845 is /far/ more
> > restricted than the general problem.
>
> It doesn't sound to me like implementing platform-specific workarounds
> is a good long-term solution (especially since, according to Bjorn, they
> aren't as trivial to implement as it sounds). And we already have all
> the infrastructure in place to implement what you describe, so I don't
> see why we shouldn't do that. This patchset uses standard device tree
> bindings that were designed for exactly this kind of use-case.
>
I think my results would imply that we would have to end up with (at
least) some special case of your proposal (i.e. we need a context bank
allocated).
> So at least for device-tree based boot firmware can already describe
> these pre-existing mappings. If something standard materializes for ACPI
> eventually I'm sure we can find ways to integrate that into whatever we
> come up with now for DT.
>
> I think between Bjorn, John, Laurentiu and myself there's pretty broad
> consensus (correct me if I'm wrong, guys) that solving this via reserved
> memory regions is a good solution that works. So I think what's really
> missing is feedback on whether the changes proposed here or Laurentiu's
> updated proposal[0] are acceptable, and if not, what the preference is
> for getting something equivalent upstream.
>
As described in my reply to your proposal, the one problem I ran into
was that I haven't figured out how to reliably "move" my display streams
from one mapping entry to another.
With the current scheme I see that their will either be gaps in time
with no mapping for my display, or multiple mappings.
The other thing I noticed in your proposal was that I have a whole bunch
of DT nodes with both iommus and memory-region properties that I really
don't care to set up mappings for, but I've not finalized my thoughts on
this causing actual problems...
> Just to highlight: the IOMMU framework already provides infrastructure
> to create direct mappings (via iommu_get_resv_regions(), called from
> iommu_create_device_direct_mappings()). I have patches that make use of
> this on Tegra210 and earlier where a non-ARM SMMU is used and where the
> IOMMU driver enables translations (and doesn't fault by default) only at
> device attachment time. That works perfectly using reserved-memory
> regions. Perhaps that infrastructure could be extended to cover the
> kinds of early mappings that we're discussing here. On the other hand it
> might be a bit premature at this point because the ARM SMMU is the only
> device that currently needs this, as far as I can tell.
>
For Qualcomm we got patches picked up for 5.8 that will cause the
display controller to be attached with direct mapping, so I think all
missing now is the lack of stream mappings between arm-smmu probe and
display driver probe...
Regards,
Bjorn
> Thierry
>
> [0]: https://patchwork.ozlabs.org/project/linux-tegra/list/?series=164853
>
> > Could you please try hacking something along the following lines and see
> > how you get on? You may need my for-joerg/arm-smmu/updates branch for
> > all the pieces:
> >
> > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
> > "pinning" and configure for bypass.
> >
> > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
> > for the display controller
> >
> > I /think/ that's sufficient, but note that it differs from the current
> > approach because we don't end up reserving a CB -- bypass is configured
> > in the S2CR instead. Some invalidation might therefore be needed in
> > ->cfg_probe() after unhooking the CB.
> >
> > Thanks, and please yell if you run into problems with this approach.
> >
> > Will
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-06-02 18:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
patches-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA, Rafael J . Wysocki,
Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio
In-Reply-To: <20200602175241.GO89269@dtor-ws>
Hi Dmitry,
W dniu 02.06.2020 o 19:52, Dmitry Torokhov pisze:
> Hi Andrzej,
>
> On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
>> Hi Dmitry,
>>
>> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
>>> That said, I think the way we should handle inhibit/uninhibit, is that
>>> if we have the callback defined, then we call it, and only call open and
>>> close if uninhibit or inhibit are _not_ defined.
>>>
>>
>> If I understand you correctly you suggest to call either inhibit,
>> if provided or close, if inhibit is not provided, but not both,
>> that is, if both are provided then on the inhibit path only
>> inhibit is called. And, consequently, you suggest to call either
>> uninhibit or open, but not both. The rest of my mail makes this
>> assumption, so kindly confirm if I understand you correctly.
>
> Yes, that is correct. If a driver wants really fine-grained control, it
> will provide inhibit (or both inhibit and close), otherwise it will rely
> on close in place of inhibit.
>
>>
>> In my opinion this idea will not work.
>>
>> The first question is should we be able to inhibit a device
>> which is not opened? In my opinion we should, in order to be
>> able to inhibit a device in anticipation without needing to
>> open it first.
>
> I agree.
>
>>
>> Then what does opening (with input_open_device()) an inhibited
>> device mean? Should it succeed or should it fail?
>
> It should succeed.
>
>> If it is not
>> the first opening then effectively it boils down to increasing
>> device's and handle's counters, so we can allow it to succeed.
>> If, however, the device is being opened for the first time,
>> the ->open() method wants to be called, but that somehow
>> contradicts the device's inhibited state. So a logical thing
>> to do is to either fail input_open_device() or postpone ->open()
>> invocation to the moment of uninhibiting - and the latter is
>> what the patches in this series currently do.
>>
>> Failing input_open_device() because of the inhibited state is
>> not the right thing to do. Let me explain. Suppose that a device
>> is already inhibited and then a new matching handler appears
>> in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
>> mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
>> any character devices (only evdev.c, joydev.c and mousedev.c do),
>> so for them it makes no sense to delay calling input_open_device()
>> and it is called in handler's ->connect(). If input_open_device()
>> now fails, we have lost the only chance for this ->connect() to
>> succeed.
>>
>> Summarizing, IMO the uninhibit path should be calling both
>> ->open() and ->uninhibit() (if provided), and conversely, the inhibit
>> path should be calling both ->inhibit() and ->close() (if provided).
>
> So what you are trying to say is that you see inhibit as something that
> is done in addition to what happens in close. But what exactly do you
> want to do in inhibit, in addition to what close is doing?
See below (*).
>
> In my view, if we want to have a dedicated inhibit callback, then it
> will do everything that close does, they both are aware of each other
> and can sort out the state transitions between them. For drivers that do
> not have dedicated inhibit/uninhibit, we can use open and close
> handlers, and have input core sort out when each should be called. That
> means that we should not call dev->open() in input_open_device() when
> device is inhibited (and same for dev->close() in input_close_device).
> And when uninhibiting, we should not call dev->open() when there are no
> users for the device, and no dev->close() when inhibiting with no users.
>
> Do you see any problems with this approach?
My concern is that if e.g. both ->open() and ->uninhibit() are provided,
then in certain circumstances ->open() won't be called:
1. users == 0
2. inhibit happens
3. input_open_device() happens, ->open() not called
4. uninhibit happens
5. as part of uninhibit ->uninhibit() is only called, but ->open() is not.
They way I understand your answer is that we implicitly impose requirements
on drivers which choose to implement e.g. both ->open() and ->uninhibit():
in such a case ->uninhibit() should be doing exactly the same things as
->open() does. Which leads to a conclusion that in practice no drivers
should choose to implement both, otherwise they must be aware that
->uninhibit() can be sometimes called instead of ->open(). Then ->open()
becomes synonymous with ->uninhibit(), and ->close() with ->inhibit().
Or, maybe, then ->inhibit() can be a superset of ->close() and
->uninhibit() a superset of ->open().
If such an approach is ok with you, it is ok with me, too.
(*)
Calling both ->inhibit() and ->close() (if they are provided) allows
drivers to go fancy and fail inhibiting (which is impossible using
only ->close() as it does not return a value, but ->inhibit() by design
does). Then ->uninhibit() is mostly for symmetry.
Regards,
Andrzej
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Dmitry Torokhov @ 2020-06-02 17:52 UTC (permalink / raw)
To: Andrzej Pietrasiewicz
Cc: Hans de Goede, linux-input, linux-acpi, linux-iio,
linux-arm-kernel, linux-samsung-soc, linux-tegra, patches,
ibm-acpi-devel, platform-driver-x86, Rafael J . Wysocki,
Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio
In-Reply-To: <88f939cd-1518-d516-59f2-8f627a6a70d2@collabora.com>
Hi Andrzej,
On Tue, Jun 02, 2020 at 06:56:40PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Dmitry,
>
> W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
> > That said, I think the way we should handle inhibit/uninhibit, is that
> > if we have the callback defined, then we call it, and only call open and
> > close if uninhibit or inhibit are _not_ defined.
> >
>
> If I understand you correctly you suggest to call either inhibit,
> if provided or close, if inhibit is not provided, but not both,
> that is, if both are provided then on the inhibit path only
> inhibit is called. And, consequently, you suggest to call either
> uninhibit or open, but not both. The rest of my mail makes this
> assumption, so kindly confirm if I understand you correctly.
Yes, that is correct. If a driver wants really fine-grained control, it
will provide inhibit (or both inhibit and close), otherwise it will rely
on close in place of inhibit.
>
> In my opinion this idea will not work.
>
> The first question is should we be able to inhibit a device
> which is not opened? In my opinion we should, in order to be
> able to inhibit a device in anticipation without needing to
> open it first.
I agree.
>
> Then what does opening (with input_open_device()) an inhibited
> device mean? Should it succeed or should it fail?
It should succeed.
> If it is not
> the first opening then effectively it boils down to increasing
> device's and handle's counters, so we can allow it to succeed.
> If, however, the device is being opened for the first time,
> the ->open() method wants to be called, but that somehow
> contradicts the device's inhibited state. So a logical thing
> to do is to either fail input_open_device() or postpone ->open()
> invocation to the moment of uninhibiting - and the latter is
> what the patches in this series currently do.
>
> Failing input_open_device() because of the inhibited state is
> not the right thing to do. Let me explain. Suppose that a device
> is already inhibited and then a new matching handler appears
> in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
> mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
> any character devices (only evdev.c, joydev.c and mousedev.c do),
> so for them it makes no sense to delay calling input_open_device()
> and it is called in handler's ->connect(). If input_open_device()
> now fails, we have lost the only chance for this ->connect() to
> succeed.
>
> Summarizing, IMO the uninhibit path should be calling both
> ->open() and ->uninhibit() (if provided), and conversely, the inhibit
> path should be calling both ->inhibit() and ->close() (if provided).
So what you are trying to say is that you see inhibit as something that
is done in addition to what happens in close. But what exactly do you
want to do in inhibit, in addition to what close is doing?
In my view, if we want to have a dedicated inhibit callback, then it
will do everything that close does, they both are aware of each other
and can sort out the state transitions between them. For drivers that do
not have dedicated inhibit/uninhibit, we can use open and close
handlers, and have input core sort out when each should be called. That
means that we should not call dev->open() in input_open_device() when
device is inhibited (and same for dev->close() in input_close_device).
And when uninhibiting, we should not call dev->open() when there are no
users for the device, and no dev->close() when inhibiting with no users.
Do you see any problems with this approach?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-06-02 16:56 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
patches-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA, Rafael J . Wysocki,
Len Brown, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Kukjin Kim, Krzysztof Kozlowski,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP
In-Reply-To: <20200527063430.GJ89269@dtor-ws>
Hi Dmitry,
W dniu 27.05.2020 o 08:34, Dmitry Torokhov pisze:
> On Tue, May 19, 2020 at 11:36:34AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 5/19/20 11:02 AM, Andrzej Pietrasiewicz wrote:
>>> Hi Hans, Hi Dmitry,
>>>
>>> W dniu 18.05.2020 o 16:23, Hans de Goede pisze:
>>>> Hi,
>>>
>>> <snip>
>>>
>>>>>>>>
>>>>>>>> So I wonder what this series actually adds for functionality for
>>>>>>>> userspace which can not already be achieved this way?
>>>>>>>>
>>>>>>>> I also noticed that you keep the device open (do not call the
>>>>>>>> input_device's close callback) when inhibited and just throw away
>>>>>>>
>>>>>>> I'm not sure if I understand you correctly, it is called:
>>>>>>>
>>>>>>> +static inline void input_stop(struct input_dev *dev)
>>>>>>> +{
>>>>>>> + if (dev->poller)
>>>>>>> + input_dev_poller_stop(dev->poller);
>>>>>>> + if (dev->close)
>>>>>>> + dev->close(dev);
>>>>>>> ^^^^^^^^^^^^^^^^
>>>>>>> +static int input_inhibit(struct input_dev *dev)
>>>>>>> +{
>>>>>>> + int ret = 0;
>>>>>>> +
>>>>>>> + mutex_lock(&dev->mutex);
>>>>>>> +
>>>>>>> + if (dev->inhibited)
>>>>>>> + goto out;
>>>>>>> +
>>>>>>> + if (dev->users) {
>>>>>>> + if (dev->inhibit) {
>>>>>>> + ret = dev->inhibit(dev);
>>>>>>> + if (ret)
>>>>>>> + goto out;
>>>>>>> + }
>>>>>>> + input_stop(dev);
>>>>>>> ^^^^^^^^^^^^^^^^
>>>>>>>
>>>>>>> It will not be called when dev->users is zero, but if it is zero,
>>>>>>> then nobody has opened the device yet so there is nothing to close.
>>>>>>
>>>>>> Ah, I missed that.
>>>>>>
>>>>>> So if the device implements the inhibit call back then on
>>>>>> inhibit it will get both the inhibit and close callback called?
>>>>>>
>>>>>
>>>>> That's right. And conversely, upon uninhibit open() and uninhibit()
>>>>> callbacks will be invoked. Please note that just as with open()/close(),
>>>>> providing inhibit()/uninhibit() is optional.
>>>>
>>>> Ack.
>>>>
>>>>>> And what happens if the last user goes away and the device
>>>>>> is not inhibited?
>>>>>
>>>>> close() is called as usually.
>>>>
>>>> But not inhibit, hmm, see below.
>>>>
>>>>>> I'm trying to understand here what the difference between the 2
>>>>>> is / what the goal of having a separate inhibit callback ?
>>>>>>
>>>>>
>>>>> Drivers have very different ideas about what it means to suspend/resume
>>>>> and open/close. The optional inhibit/uninhibit callbacks are meant for
>>>>> the drivers to know that it is this particular action going on.
>>>>
>>>> So the inhibit() callback triggers the "suspend" behavior ?
>>>> But shouldn't drivers which are capable of suspending the device
>>>> always do so on close() ?
>>>>
>>>> Since your current proposal also calls close() on inhibit() I
>>>> really see little difference between an inhibit() and the last
>>>> user of the device closing it and IMHO unless there is a good
>>>> reason to actually differentiate the 2 it would be better
>>>> to only stick with the existing close() and in cases where
>>>> that does not put the device in a low-power mode yet, fix
>>>> the existing close() callback to do the low-power mode
>>>> setting instead of adding a new callback.
>>>>
>>>>> For inhibit() there's one more argument: close() does not return a value,
>>>>> so its meaning is "do some last cleanup" and as such it is not allowed
>>>>> to fail - whatever its effect is, we must deem it successful. inhibit()
>>>>> does return a value and so it is allowed to fail.
>>>>
>>>> Well, we could make close() return an error and at least in the inhibit()
>>>> case propagate that to userspace. I wonder if userspace is going to
>>>> do anything useful with that error though...
>
> It really can't do anything. Have you ever seen userspace handling
> errors from close()? And what can be done? A program is terminating, but
> the kernel says "no, you closing input device failed, you have to
> continue running indefinitely..."
>
>>>>
>>>> In my experience errors during cleanup/shutdown are best logged
>>>> (using dev_err) and otherwise ignored, so that we try to clean up
>>>> as much possible. Unless the very first step of the shutdown process
>>>> fails the device is going to be in some twilight zone state anyways
>>>> at this point we might as well try to cleanup as much as possible.
>>>
>>> What you say makes sense to me.
>>> @Dmitry?
>
> I will note here, that inhibit is closer to suspend() than to close(),
> and we do report errors for suspend(). Therefore we could conceivably
> try to handle errors if driver really wants to be fancy. But I think
> majority of cases will be quite happy with using close() and simply
> logging errors, as Hans said.
>
> That said, I think the way we should handle inhibit/uninhibit, is that
> if we have the callback defined, then we call it, and only call open and
> close if uninhibit or inhibit are _not_ defined.
>
If I understand you correctly you suggest to call either inhibit,
if provided or close, if inhibit is not provided, but not both,
that is, if both are provided then on the inhibit path only
inhibit is called. And, consequently, you suggest to call either
uninhibit or open, but not both. The rest of my mail makes this
assumption, so kindly confirm if I understand you correctly.
In my opinion this idea will not work.
The first question is should we be able to inhibit a device
which is not opened? In my opinion we should, in order to be
able to inhibit a device in anticipation without needing to
open it first.
Then what does opening (with input_open_device()) an inhibited
device mean? Should it succeed or should it fail? If it is not
the first opening then effectively it boils down to increasing
device's and handle's counters, so we can allow it to succeed.
If, however, the device is being opened for the first time,
the ->open() method wants to be called, but that somehow
contradicts the device's inhibited state. So a logical thing
to do is to either fail input_open_device() or postpone ->open()
invocation to the moment of uninhibiting - and the latter is
what the patches in this series currently do.
Failing input_open_device() because of the inhibited state is
not the right thing to do. Let me explain. Suppose that a device
is already inhibited and then a new matching handler appears
in the system. Most handlers (apm-power.c, evbug.c, input-leds.c,
mac_hid.c, sysrq.c, vt/keyboard.c and rfkill/input.c) don't create
any character devices (only evdev.c, joydev.c and mousedev.c do),
so for them it makes no sense to delay calling input_open_device()
and it is called in handler's ->connect(). If input_open_device()
now fails, we have lost the only chance for this ->connect() to
succeed.
Summarizing, IMO the uninhibit path should be calling both
->open() and ->uninhibit() (if provided), and conversely, the inhibit
path should be calling both ->inhibit() and ->close() (if provided).
What's your opinion?
Regards,
Andrzej
^ permalink raw reply
* Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
From: Jerry Snitselaar @ 2020-06-02 16:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: Lu Baolu, Will Deacon, Robin Murphy, Marek Szyprowski, Kukjin Kim,
Krzysztof Kozlowski, David Woodhouse, Andy Gross, Bjorn Andersson,
Matthias Brugger, Rob Clark, Heiko Stuebner, Gerald Schaefer,
Thierry Reding, Jonathan Hunter, Jean-Philippe Brucker,
linux-s390, linux-samsung-soc, linux-arm-msm, linux-kernel,
virtualiza
In-Reply-To: <20200602142312.GJ14598@8bytes.org>
On Tue Jun 02 20, Joerg Roedel wrote:
>Hi Jerry,
>
>On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
>>
>> Yeah, that will solve the panic.
>>
>
>If you still see the kdump faults, can you please try with the attached
>diff? I was not able to reproduce them in my setup.
>
>Regards,
>
> Joerg
>
I have another hp proliant server now, and reproduced. I will have the
patch below tested shortly. Minor change, I switched group->domain to
domain since group isn't an argument, and *data being passed in comes
from group->domain anyways.
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index b5ea203f6c68..5a6d509f72b6 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
> static int iommu_group_do_dma_attach(struct device *dev, void *data)
> {
> struct iommu_domain *domain = data;
>+ int ret = 0;
>
>- return __iommu_attach_device(domain, dev);
>+ if (!iommu_is_attach_deferred(group->domain, dev))
>+ ret = __iommu_attach_device(group->domain, dev);
>+
>+ return ret;
> }
>
> static int __iommu_group_dma_attach(struct iommu_group *group)
>_______________________________________________
>iommu mailing list
>iommu@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox