Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682145 > unrolled thread

Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression Boszormenyi Zoltan <zboszor@pr.hu> - 2017-07-06 10:00 +0200
    Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression "Marcelo \"Marc\" Ranolfi" <marc.2377@gmail.com> - 2017-07-07 04:30 +0200

#1682145 — Re: [PATCH 0/5 v4] Fix sp5100_tco watchdog driver regression

Hi,

ping for the series.

Adding Greg Kroah-Hartman to the cc: list, both for the USB core
and stable series maintainership.

2017-06-22 15:21 keltezéssel, Zoltán Böszörményi írta:
> This patch series fixes a regression introduced by:
> 
> commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
> Author: Christian Fetzer <fetzer.ch@gmail.com>
> Date:   Thu Nov 19 20:13:48 2015 +0100
> 
>      i2c: piix4: Add support for multiplexed main adapter in SB800
> 
> The regression caused sp5100_tco fail to load:
> 
> sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
> sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
> sp5100_tco: I/O address 0x0cd6 already in use
> 
> Notable bugzilla links about this issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=170741
> https://bugzilla.redhat.com/show_bug.cgi?id=1369269
> https://bugzilla.redhat.com/show_bug.cgi?id=1406844
> 
> The previous two versions of this patch series introduced
> a common mutex to synchronize access to the I/O port pair
> 0xcd6 / 0xcd7 used by the AMD SB800 USB PCI quirk code and
> the i2c-piix and sp5100_tco drivers. The common mutex was
> criticized because it introduces an inter-dependency between
> drivers.
> 
> This approach modifies the request_muxed_region() semantics and
> modifies the possible use cases.
> 
> The first patch in the series adds a new IORESOURCE_ALLOCATED
> flag that alloc_resource() sets and free_resource() considers.
> The core of __request_region() is factored out into a new function
> that doesn't allocate. With this change, drivers can use the
> pre-existing DEFINE_RES_IO_NAMED() static initialized macro
> to declare struct resource statically (e.g. on the stack)
> and pass the address of it to the new __request_declared_region()
> function. A new macro called request_declared_muxed_region()
> was added to exploit this functionality. Because of the new
> IORESOURCE_ALLOCATED resource flag, release_region() can still
> be called with the old interface (the port region start and
> end values) and it won't attempt to free a non-allocated resource.
> This eliminated one failure case that can come from allocation
> errors.
> 
> The second patch modifies the behaviour of IORESOURCE_MUXED,
> a.k.a. the request_*muxed_region() macros. When these macros
> are called, the caller goes to sleep when there is any conflicting
> regions, even if the conflicting region did not use the
> IORESOURCE_MUXED flag. The kernel logs this inconsistent
> flag usage with KERN_ERR. This change eliminates the second
> failure case for IORESOURCE_MUXED and request_muxed_region()
> can be used like mutex_lock(), i.e. it returns only in case it
> could successfully request the region.
> 
> The last three patches adds proper synchronization between the
> USB PCI quirks code and the i2c-piix and sp5100_tco drivers.
> 
> The result is that the sp5100_tco driver can load and works again:
> 
> sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
> sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
> sp5100_tco: Using 0xfed80b00 for watchdog MMIO address
> sp5100_tco: Last reboot was not triggered by watchdog.
> sp5100_tco: initialized (0xffffba2f4192db00). heartbeat=60 sec (nowayout=0)
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/i2c/busses/i2c-piix4.c | 41 ++++++++++++-------------------------
>   drivers/usb/host/pci-quirks.c  |  4 ++++
>   drivers/watchdog/sp5100_tco.c  | 28 +++++++++++++------------
>   include/linux/ioport.h         | 14 +++++++++++++
>   kernel/resource.c              | 46 ++++++++++++++++++++++++++++++++++++++----
>   5 files changed, 88 insertions(+), 45 deletions(-)
> 
> 

The synchronized access to the SB800 I/O ports seems to also have made a rare
"disabled by hub (EMI?), re-enabling..." report from the kernel disappear.

Can someone review the series?

Thanks in advance,
Zoltán Böszörményi

[toc] | [next] | [standalone]

#1682878

(Reposting without html subpart, second attempt)

On Thu, Jul 6, 2017 at 11:09 PM, Marcelo "Marc" Ranolfi
<marc.2377@gmail.com> wrote:
Hi Zoltán and all,

I _was_ testing the patch series (the latest version), system worked
OK for two days, but then I got a system crash (probably hardware
related) and lost my files in the main partition. Posted about it in
the F2FS mailing list
(https://sourceforge.net/p/linux-f2fs/mailman/message/35928135/).

While I don't believe that the freeze/crash was caused by the watchdog
driver (many other things to blame: amdgpu driver on Southern Islands,
CPU voltage lower than recommended...), it's worth mentioning that the
computer was completely frozen for a little over 4 minutes, and did
not reset as I'd expect the watchdog to. Just mentioning because I'm
not sure of the implications.

Once I get my system back online this weekend I'll have a good look at
the code. Anyway, thanks again for the patches and I'm also expecting
someone more qualified (than me, at least) to review them.

Regards
Marcelo "Marc" Ranolfi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web