Re: [PATCH net v2 3/4] net: mana: Don't overwrite port probe error with add_adev result

[Erni Sri Satya Vennela]

On Tue, Apr 14, 2026 at 04:35:03PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@xxxxxxxxxx>
> 
> 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
> ---
> net: mana: Don't overwrite port probe error with add_adev result
> 
> This commit ensures that if mana_probe_port() or mana_attach() fails, the
> error is not overwritten by the result of add_adev(). By preserving the
> error, it triggers mana_remove() cleanup instead of leaving ports partially
> initialized.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 9d095a6fb56c7..4d42cbfb3f15b 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -3634,10 +3634,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> >  	if (!resuming) {
> >  		for (i = 0; i < ac->num_ports; i++) {
> >  			err = mana_probe_port(ac, i, &ac->ports[i]);
> > -			/* we log the port for which the probe failed and stop
> > -			 * probes for subsequent ports.
> > -			 * Note that we keep running ports, for which the probes
> > -			 * were successful, unless add_adev fails too
> > +			/* Log the port for which the probe failed, stop probing
> > +			 * subsequent ports, and skip add_adev.
> > +			 * Already-probed ports remain functional.
> >  			 */
> 
> Are these comments accurate regarding ports remaining functional?
> 
> Since this patch preserves the error code, execution falls through to the end
> of mana_probe() where it checks if err is set and calls mana_remove().
> mana_remove() will iterate over the successfully allocated ports, completely
> detach them, unregister their net_devices, and destroy event queues.
> 
> It appears no ports remain functional because the device is completely torn
> down.
>
I will update the patch with the updated comment message. 
> [ ... ]
> 
> > @@ -3651,10 +3650,9 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> >  			enable_work(&apc->queue_reset_work);
> >  			err = mana_attach(ac->ports[i]);
> >  			rtnl_unlock();
> > -			/* we log the port for which the attach failed and stop
> > -			 * attach for subsequent ports
> > -			 * Note that we keep running ports, for which the attach
> > -			 * were successful, unless add_adev fails too
> > +			/* Log the port for which the attach failed, stop
> > +			 * attaching subsequent ports, and skip add_adev.
> > +			 * Already-attached ports remain functional.
> >  			 */
> >  			if (err) {
> >  				dev_err(dev, "Attach Failed for port %d\n", i);
> > @@ -3663,7 +3661,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> 
> Does propagating this error during PM resume introduce a NULL pointer
> dereference?
> 
> If PM resume fails here (resuming == true), the preserved error code causes
> mana_probe() to call mana_remove(), which tears down the device and sets
> gd->gdma_context = NULL.
> 
> However, a failed resume callback does not automatically unbind the driver.
> When the device is eventually unbound later, mana_remove() is invoked again:
> 
> drivers/net/ethernet/microsoft/mana/mana_en.c:mana_remove() {
>     struct gdma_context *gc = gd->gdma_context;
>     struct device *dev = gc->dev;
>     ...
> }
> 
> In the second invocation, gd->gdma_context is NULL, meaning gc is NULL,
> which could lead to a kernel panic when dereferencing gc->dev.

Thankyou for pointing it out, Simon.
Since this is a pre-existing bug, I will create a different patch for
this change and make it as part of this patchset.

- Vennela