Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Asmaa Mnebhi <asmaa@nvidia.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     Linus Walleij <linus.walleij@linaro.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        David Thompson <davthompson@nvidia.com>
Subject: RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Topic: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Index: AQHXsLi5/ezHFM2t80qMnoiP1wWjOauzEi0AgADJmACABBI7EIAAAriAgAACbtCAAAsjgIAAAWRQgABFnwCAAxBt0A==
Date:   Wed, 29 Sep 2021 19:14:48 +0000
Message-ID: <CH2PR12MB3895BD75A1BD0048A93B4A51D7A99@CH2PR12MB3895.namprd12.prod.outlook.com>
References: <20210923202216.16091-1-asmaa@nvidia.com>
 <20210923202216.16091-2-asmaa@nvidia.com> <YU26lIUayYXU/x9l@lunn.ch>
 <CACRpkdbUJF6VUPk9kCMPBvjeL3frJAbHq+h0-z7P-a1pSU+fiw@mail.gmail.com>
 <CH2PR12MB38951F2326196AB5B573A73DD7A79@CH2PR12MB3895.namprd12.prod.outlook.com>
 <YVHQQcv2M6soJR6u@lunn.ch>
 <CH2PR12MB389585F7D5EFE5E2453593DBD7A79@CH2PR12MB3895.namprd12.prod.outlook.com>
 <YVHbo/cJcHzxUk+d@lunn.ch>
 <CH2PR12MB3895E69636DDB3811C0EAE3DD7A79@CH2PR12MB3895.namprd12.prod.outlook.com>
 <YVIXNEmhoMW7c1S/@lunn.ch>
In-Reply-To: <YVIXNEmhoMW7c1S/@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cts3kknhtMMeEPa/3/T5rqP0Fi2xYTXUuP8OpaNEmQO2omekRlNK6IwNpQ0U?=
 =?us-ascii?Q?NNCE02t8E7kfuDd9SDSoOZpJlUwH8o/DOc3+lNYz7PZAGLlNmIGAsu/CC1To?=
 =?us-ascii?Q?cJkze3hWXuGEwIqb/B5HqOmabmb2oh1PCyyi8OXcB1csnswmXZ/DklCimRpL?=
 =?us-ascii?Q?uo7xY2r5fYwIWQeRTTqhqy3T2ptHE7WFYcBuf/Vg0hMgY6eAqG1gFyJ1eDbA?=
 =?us-ascii?Q?gEDic1ba8EF66MXpkmRjaSk6acpO7KMr3jyuSLhweAQwnjNrn03R5+pMgFsj?=
 =?us-ascii?Q?bmuhkcutVXs/n12ljCWIiCWOvDBIep1DQ/Ch62hnjLvAuNJYPBDyGa/7AT+a?=
 =?us-ascii?Q?etx1t8TgRJPCIQvyXUTqOk3zJz7v0QOttYutwEKEadCQ59bjtjCbC5Vustzb?=
 =?us-ascii?Q?ZF1zNp9EL6v7Y5ZS3d1D0yfkk76zDB5rb5M0xl5aLCgdxwtNxEMkkHrE5X5x?=
 =?us-ascii?Q?3qu84+VnCb0J0Rlumz5cO9Sg5QcUbGAGI6NvuqNgGzzAdgRGfzsZs4aGyM9u?=
 =?us-ascii?Q?ujRQfL7vkxr8GaEtAx6JtHFX1UT3KjR3RPWI3vVpWre08+jsoSQF1f2nGd4h?=
 =?us-ascii?Q?YDCVHQCM0id8E2eSQ7gOmIfmv9vjyK99vm91JpeuP6EN9oYBbMXj6AP9oxYV?=
 =?us-ascii?Q?vidAz/Rkkxq8yqlnUFucIHOPONSyA4QpHbXkGEruG+AjTH+vqYmSccvGy/RC?=
 =?us-ascii?Q?GwXmkpn6xMkgUYiUYL3IaJCkEn04PYt7XN9eYxLpms67BPyY+3l8aoQoeIwA?=
 =?us-ascii?Q?XjGy5A6azXYys0siZ5NBUiGmfX/TLvK7HlewFkWWYaOz5JgVMy0sY7hhdS4s?=
 =?us-ascii?Q?uz3ZI4QSeNonnrHgvMM129KujT2RvQZb9yEXoyoYvDg1J9HOrYjGnwBBXQQH?=
 =?us-ascii?Q?dimyo0cpFCY7fmwF59+FApOczufym+Sg9VXiPE+ifbuO6DMQB8Qt8iRYk05n?=
 =?us-ascii?Q?joomQQF11j3wAUN291YY6tuyZ7GlXOJO1TGDdNrzZVWbES+CavCxmq9QJJQj?=
 =?us-ascii?Q?gsHCxQWVWNSrl/2gWUR6eXt2ghpShkZ3WxwZkhM/Eqh3RwfIp9nqLNy06Q1B?=
 =?us-ascii?Q?vesg1+b61ZjAqhGU76I24C80kS6C5luIMzYWTA8OHh8DqXmUGL6I9vtoY4an?=
 =?us-ascii?Q?dDwrO+w8hK5X8BqFpFlNEjeISsUuCNHD5bVkBbKWDcT6jA1eel7tuIqLfCzV?=
 =?us-ascii?Q?KTLiI/+/bgoJ/XoxxcIutq8bcV2wxbMdukllja+xQppOLdcTsfeT12bRkAfi?=
 =?us-ascii?Q?rqrkbrfu8/Sag8fckdbUhUkEuwXwtgkEN3Yag90QECKBZW9uaGuEb7Bdps4X?=
 =?us-ascii?Q?X/FXli5Fo+BIZVDQdpwN4GVj?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB3895.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9407d020-c849-4b8f-5a41-08d9837d6799
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Sep 2021 19:14:48.0550
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 4Y83w33/2CJmK487zDo74lfd9wbc9ZfapUQNM/3RD3syeypA7xCSBtcm1anLL1/vBpFXa65ZU1Da+mwFSFOWoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4038
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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

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

best way to trigger it is going to bring the link down as soon as it goes u=
p.

 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 interr=
upts.=20

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=20
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 interr=
upt.
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 =3D kszphy_handle_interrupt, which reads
MII_KSZPHY_INTCS regs and hence clears all interrupts at once.=20

- 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. T=
he
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.

Problem:
- however, if there is a second interrupt right before or during the MDIO r=
ead 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 no=
t
support LEVEL interrupts on the GPIOs at the moment.
They suggested to read the GPIO pin value to check if it has returned to hi=
gh
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 de=
al
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=20
> interrupt but rather a HW interrupt which is common to all GPIO pins=20
> (irrelevant here, but this is edge triggered):
> ret =3D 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 usin=
g edges.

      Andrew