Re: [PATCH net] net: ibm: emac: mal: fix potential system hang in mal_remove()

[Jacob Keller]

On 6/8/2026 6:51 PM, Rosen Penev wrote:
> On Mon, Jun 8, 2026 at 6:14 PM Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
>>
>> On 6/8/2026 5:36 PM, Jakub Kicinski wrote:
>>> On Thu, 4 Jun 2026 16:03:43 -0700 Rosen Penev wrote:
>>>>>> -     if (!list_empty(&mal->list))
>>>>>> +     if (!list_empty(&mal->list)) {
>>>>>> +             napi_disable(&mal->napi);
>>>>>>               /* This is *very* bad */
>>>>>>               WARN(1, KERN_EMERG
>>>>>>                      "mal%d: commac list is not empty on remove!\n",
>>>>>>                      mal->index);
>>>>>
>>>>> This one doesn't make sense to me. The list_empty check does a WARN()
>>>>> indicating that this is not supposed to happen.
>>>>>
>>>>> This implies that list_empty should be true, otherwise we'd see a WARN
>>>>> every time mal_remove is called.
>>>>>
>>>>> But in that case, we'd have been calling napi_disable incorrectly in
>>>>> most cases where it was previously unsafe according to your claim.
>>>>>
>>>>> At best, this list_empty check is the wrong way to tell if the napi is
>>>>> disabled, at worst, this whole change is pointless.
>>>> I asked the AI. It doesn't agree:
>>>
>>> FTR I agree with Jake, the patch seems to indicate bigger structural
>>> issues. Then again I don't want to encourage the stream of patches
>>> to this driver so let me just apply this..
>>
>> I don't see how applying this discourages the stream of patches?
>> Wouldn't that be encouraging?
>>
>> Or I guess the patch "does" fix something (an extra call to
>> napi_disable) and you don't want to encourage a raff of future changes
>> to try and fix the overall structural issues? I guess that makes sense.
>>
>> I still don't follow how this fixes
> I don't either. But runtime testing shows it to be an improvement.
> 

If you have runtime testing to back this up, then please ignore my
objections :)

I think I'd be happier with just the removal of napi_disable from
mal_remove(), and not trying to modify the list_empty WARN block, but
thats only because it does seem obvious that the existing napi_disable
shouldn't be called there (as it was called when the last item was
removed from mal->list).

> Anyway, it seems various subsystem maintainers want me to fix
> pre-existing issues reported by sashiko.
> 
> I also got this feedback previously:
> https://lore.kernel.org/netdev/20260107185605.7932173f@xxxxxxxxxx/