archive mirror
 help / color / mirror / Atom feed
From: Asmaa Mnebhi <>
To: Andrew Lunn <>
Cc: Linus Walleij <>,
	Andy Shevchenko <>,
	"open list:GPIO SUBSYSTEM" <>,
	netdev <>,
	linux-kernel <>,
	ACPI Devel Maling List <>,
	Jakub Kicinski <>,
	Bartosz Golaszewski <>,
	"David S. Miller" <>,
	"Rafael J. Wysocki" <>,
	David Thompson <>
Subject: RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support
Date: Wed, 29 Sep 2021 19:14:48 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <YVIXNEmhoMW7c1S/>

> Asmaa>> Thank you very much for the detailed and clear explanation!
> we only enable/support link up/down interrupts. QA has tested bringing 
> up/down the network interface +200 times in a loop.

The micrel driver currently only uses two interrupts of the available 8. 
So it will be hard to trigger the problem with the current driver. Your 

best way to trigger it is going to bring the link down as soon as it goes up.

 So you get first a link up, and then a link down very shortly afterwards.

There is however nothing stopping developers making use of the other interrupts. 

That will then increase the likelihood of problems.

What does help you is that the interrupt register is clear on read. So the race condition 
window is small.

Asmaa>> Hi Andrew,

I had a meeting today with the HW folks to explain the problem at stake.
The flow for this issue is like this:
1) PHY issues INT_N signal (active low level interrupt)
2) falling edge detected on the GPIO and transmitted to software
3) the first thing mlxbf2_gpio_irq_handler does is to clear the GPIO interrupt.
However even if we clear the GPIO interrupt, the GPIO value itself
will be low as long as the INT_N signal is low. The GPIO HW triggers
the interrupt by detecting the falling edge of the GPIO pin.
4) mlxbf2_gpio_irq_handler triggers phy_interrupt which
calls drv->handler_interrupt.
handle_interrupt in our case = kszphy_handle_interrupt, which reads
MII_KSZPHY_INTCS regs and hence clears all interrupts at once. 

- if no other interrupt happens within this time frame, INT_N goes
back to 1 and the next interrupt will trigger another GPIO falling edge

- if the interrupt happens after the MDIO read, then it is not a problem. The
read would have already cleared the register and INT_N would go back to 1.
So the new interrupt will trigger a new GPIO falling edge interrupt.

- however, if there is a second interrupt right before or during the MDIO read of
MII_KSZPHY_INTCS, it might not be detected by our GPIO HW.

Anyways, the HW folks agreed that this is a problem since indeed they do not
support LEVEL interrupts on the GPIOs at the moment.
They suggested to read the GPIO pin value to check if it has returned to high
in mlxbf2_gpio_irq_handler, then trigger the phy_interrupt handler.
But I don't think it is a good workaround because there could be a chain
of interrupts which hold the  LEVEL low for a long time, and we don't want to
be waiting too long in an interrupt handler routine.
I would greatly appreciate some more feedback on what is the best way to deal
With this in the upstreamed version of the driver.
HW folks said they will fix this in future BlueField generations.

> The software interrupt and handler is not registered based on the GPIO 
> interrupt but rather a HW interrupt which is common to all GPIO pins 
> (irrelevant here, but this is edge triggered):
> ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
>                                         IRQF_SHARED, name, gs);

IRQF_SHARED implied level. You cannot have a shared interrupt which is using edges.


  reply	other threads:[~2021-09-29 19:14 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-23 20:22 [PATCH v3 0/2] gpio: mlxbf2: Introduce proper interrupt handling Asmaa Mnebhi
2021-09-23 20:22 ` [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support Asmaa Mnebhi
2021-09-24 11:46   ` Andrew Lunn
2021-09-24 23:48     ` Linus Walleij
2021-09-27 14:04       ` Asmaa Mnebhi
2021-09-27 14:08         ` Andrew Lunn
2021-09-27 14:19           ` Asmaa Mnebhi
2021-09-27 14:26             ` Asmaa Mnebhi
2021-09-27 14:56             ` Andrew Lunn
2021-09-27 15:52               ` Asmaa Mnebhi
2021-09-27 19:10                 ` Andrew Lunn
2021-09-29 19:14                   ` Asmaa Mnebhi [this message]
2021-09-28 15:02               ` Asmaa Mnebhi
2021-09-29 20:24                 ` Andrew Lunn
2021-10-08 14:47                   ` Asmaa Mnebhi
2021-09-23 20:22 ` [PATCH v3 2/2] net: mellanox: mlxbf_gige: Replace non-standard interrupt handling Asmaa Mnebhi
  -- strict thread matches above, loose matches on Subject: below --
2021-09-23 20:18 [PATCH v3 0/2] gpio: mlxbf2: Introduce proper " Asmaa Mnebhi
2021-09-23 20:18 ` [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support Asmaa Mnebhi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).