Re: [PATCH 1/6] pds_core: add support for quiet devcmd failures

["Rao, Nikhil" <nikhirao@amd.com>]

On 7/16/2026 12:17 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> --- Patch [1]: [PATCH 1/6] pds_core: add support for quiet devcmd failures ---
> commit 2618e77f5d1aae31c9817f09b5056a69e237162a
> Author: Brett Creeley <brett.creeley@amd.com>
> 
> pds_core: add support for quiet devcmd failures
> 
> This commit adds support to allow devcmd failure messages to be suppressed.
> This will be used when adding support to negotiate
> PDS_CORE_IDENTITY_VERSION_2.
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c
>> index bded6b33289ce..dd9989cfe6b3f 100644
>> --- a/drivers/net/ethernet/amd/pds_core/dev.c
>> +++ b/drivers/net/ethernet/amd/pds_core/dev.c
> 
> [ ... ]
> 
>> @@ -197,7 +199,7 @@ int pdsc_devcmd_locked(struct pdsc *pdsc, union pds_core_dev_cmd *cmd,
>>
>>        memcpy_toio(&pdsc->cmd_regs->cmd, cmd, sizeof(*cmd));
>>        pdsc_devcmd_dbell(pdsc);
>> -     err = pdsc_devcmd_wait(pdsc, cmd->opcode, max_seconds);
>> +     err = __pdsc_devcmd_wait(pdsc, cmd->opcode, max_seconds, do_msg);
>>
>>        if ((err == -ENXIO || err == -ETIMEDOUT) && pdsc->wq)
>>                queue_work(pdsc->wq, &pdsc->health_work);
> 
> [Severity: High]
> This is a pre-existing issue, but does this error path trigger a
> use-after-free during device removal?
> 
> During device removal, pdsc_remove() destroys the workqueue without clearing
> the pointer before invoking pdsc_teardown():
> 
> pdsc_remove() {
>          ...
>          if (pdsc->wq)
>                  destroy_workqueue(pdsc->wq);
> 
>          mutex_lock(&pdsc->config_lock);
>          set_bit(PDSC_S_STOPPING_DRIVER, &pdsc->state);
> 
>          pdsc_stop(pdsc);
>          pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
>          ...
> }
> 
> The teardown function issues a hardware reset command via pdsc_devcmd_reset().
> If the device is unresponsive and the command times out, the error path here in
> __pdsc_devcmd_locked() catches the -ETIMEDOUT error.
> 
> Because pdsc->wq was left as a dangling pointer, the condition evaluates to true
> and execution falls through to queue_work(), which will dereference the already
> destroyed workqueue pointer.
> 
> Could the pdsc->wq pointer be set to NULL immediately after destruction to
> prevent this, or should the teardown sequence be reordered?
> 
>>        else
>>                memcpy_fromio(comp, &pdsc->cmd_regs->comp, sizeof(*comp));
>>
>>        if (err != -ETIMEDOUT && err != -EAGAIN)
>>                pdsc_deferred_dma_free(pdsc);
>>
>>        return err;
>>   }

Fixed in [1] by reordering destroy_workqueue() after pdsc_teardown().

Thanks,
Nikhil

[1] 
https://lore.kernel.org/netdev/20260714180223.1642792-3-nikhil.rao@amd.com/