netdev - Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for mse102x_rx_pkt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <923bc33f-db94-4aea-8225-9125298ecfff@gmx.net>
Date: Tue, 6 May 2025 10:24:26 +0200
From: Stefan Wahren <wahrenst@....net>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 4/5] net: vertexcom: mse102x: Return code for
 mse102x_rx_pkt_spi

Hi Andrew,

Am 05.05.25 um 21:27 schrieb Andrew Lunn:
> On Mon, May 05, 2025 at 07:16:51PM +0200, Stefan Wahren wrote:
>> Hi Andrew,
>>
>> Am 05.05.25 um 18:43 schrieb Andrew Lunn:
>>> On Mon, May 05, 2025 at 04:24:26PM +0200, Stefan Wahren wrote:
>>>> The interrupt handler mse102x_irq always returns IRQ_HANDLED even
>>>> in case the SPI interrupt is not handled. In order to solve this,
>>>> let mse102x_rx_pkt_spi return the proper return code.
>>>>
>>>> Signed-off-by: Stefan Wahren <wahrenst@....net>
>>>> ---
>>>>    drivers/net/ethernet/vertexcom/mse102x.c | 15 +++++++++------
>>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
>>>> index 204ce8bdbaf8..aeef144d0051 100644
>>>> --- a/drivers/net/ethernet/vertexcom/mse102x.c
>>>> +++ b/drivers/net/ethernet/vertexcom/mse102x.c
>>>> @@ -303,7 +303,7 @@ static void mse102x_dump_packet(const char *msg, int len, const char *data)
>>>>    		       data, len, true);
>>>>    }
>>>> -static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>> +static irqreturn_t mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    {
>>>>    	struct sk_buff *skb;
>>>>    	unsigned int rxalign;
>>>> @@ -324,7 +324,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    		mse102x_tx_cmd_spi(mse, CMD_CTR);
>>>>    		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
>>>>    		if (ret)
>>>> -			return;
>>>> +			return IRQ_NONE;
>>>>    		cmd_resp = be16_to_cpu(rx);
>>>>    		if ((cmd_resp & CMD_MASK) != CMD_RTS) {
>>>> @@ -357,7 +357,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>>>    	rxalign = ALIGN(rxlen + DET_SOF_LEN + DET_DFT_LEN, 4);
>>>>    	skb = netdev_alloc_skb_ip_align(mse->ndev, rxalign);
>>>>    	if (!skb)
>>>> -		return;
>>>> +		return IRQ_NONE;
>>> This is not my understanding of IRQ_NONE. To me, IRQ_NONE means the
>>> driver has read the interrupt status register and determined that this
>>> device did not generate the interrupt. It is probably some other
>>> device which is sharing the interrupt.
>> At first i wrote this patch for the not-shared interrupt use case in mind.
>> Unfortunately this device doesn't have a interrupt status register and in
>> the above cases the interrupt is not handled.
>>
>> kernel-doc says:
>>
>> @IRQ_NONE:        interrupt was not from this device or was not handled
>> @IRQ_HANDLED:    interrupt was handled by this device
> A memory allocation failure in netdev_alloc_skb_ip_align() does not
> seem like a reason to return IRQ_NONE. I think the more normal case
> is, there was an interrupt, there was an attempt to handle it, but the
> handler failed.
In my opinion this applies to both IRQ_NONE cases. The problem here is 
that there are no interrupt register provided by the MSE102x, so the 
only way to handle the SPI interrupt is to fetch the whole packet from 
the MSE102x internal buffer via SPI. But this requires a receive buffer 
which is big enough on the host side, so we use 
netdev_alloc_skb_ip_align() here to allocate this (SPI) receive buffer. 
The only way to avoid the second error case would be to preallocate a 
buffer which is big enough for the largest possible Ethernet frame.
>   The driver should try to put the hardware into a state
> the next interrupt will actually happen, and be serviced.
>
> This is particularly important with level interrupts. If you fail to
> clear the interrupt, it is going to fire again immediately after
> exiting the interrupt handler and the interrupt is reenabled. You
> don't want to die in an interrupt storm. Preventing such interrupt
> storms is part of what the return value is used for. If the handler
> continually returns IRQ_NONE, after a while the core declares there is
> nobody actually interested in the interrupt, and it leaves it
> disabled.
Yes, this was the intention of this patch. It's better to disable the 
IRQ completely because there is something wrong with the hardware 
instead of triggering a interrupt storm. So this is an argument to 
return IRQ_NONE and not IRQ_HANDLED.

I was dealing with interrupt storms on Raspberry Pi caused by dwc2 and 
it's a PITA.
>
>> So from my understanding IRQ_NONE fits better here (assuming a not-shared
>> interrupt).
>>
>> I think driver should only use not-shared interrupts, because there is no
>> interrupt status register. Am I right?
> I don't see why it cannot be shared. It is not very efficient, and
> there will probable be a bias towards the first device which requests
> the interrupt, but a shared interrupt should work.
I agree in general, but i don't recommend it.

Regards
>
> 	Andrew