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

[Simon Horman <horms@kernel.org>]

On Wed, Apr 15, 2026 at 12:04:51AM -0700, Erni Sri Satya Vennela wrote:
> On Tue, Apr 14, 2026 at 04:35:03PM +0100, Simon Horman wrote:
> > From: 'Simon Horman' <horms@kernel.org>
> > 
> > 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. 

Thanks.

> > [ ... ]
> > 
> > > @@ -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.

Likewise, thanks.

FTR, it it is a pre-existing bug then I don't think it needs
to block progress of your patchset. Even if fixing things
sooner than later is a good maxim.