Re: [patch net-next 02/11] mlxsw: core_linecards: Introduce per line card auxiliary device

[Ido Schimmel <idosch@nvidia.com>]

On Wed, Jun 15, 2022 at 07:37:15PM +0200, Jiri Pirko wrote:
> Wed, Jun 15, 2022 at 04:52:13PM CEST, idosch@nvidia.com wrote:
> >> +int mlxsw_linecard_bdev_add(struct mlxsw_linecard *linecard)
> >> +{
> >> +	struct mlxsw_linecard_bdev *linecard_bdev;
> >> +	int err;
> >> +	int id;
> >> +
> >> +	id = mlxsw_linecard_bdev_id_alloc();
> >> +	if (id < 0)
> >> +		return id;
> >> +
> >> +	linecard_bdev = kzalloc(sizeof(*linecard_bdev), GFP_KERNEL);
> >> +	if (!linecard_bdev) {
> >> +		mlxsw_linecard_bdev_id_free(id);
> >> +		return -ENOMEM;
> >> +	}
> >> +	linecard_bdev->adev.id = id;
> >> +	linecard_bdev->adev.name = MLXSW_LINECARD_DEV_ID_NAME;
> >> +	linecard_bdev->adev.dev.release = mlxsw_linecard_bdev_release;
> >> +	linecard_bdev->adev.dev.parent = linecard->linecards->bus_info->dev;
> >> +	linecard_bdev->linecard = linecard;
> >> +
> >> +	err = auxiliary_device_init(&linecard_bdev->adev);
> >> +	if (err) {
> >> +		mlxsw_linecard_bdev_id_free(id);
> >> +		kfree(linecard_bdev);
> >> +		return err;
> >> +	}
> >> +
> >> +	err = auxiliary_device_add(&linecard_bdev->adev);
> >> +	if (err) {
> >> +		auxiliary_device_uninit(&linecard_bdev->adev);
> >> +		return err;
> >> +	}
> >> +
> >> +	linecard->bdev = linecard_bdev;
> >> +	return 0;
> >> +}
> >
> >[...]
> >
> >> +static int mlxsw_linecard_bdev_probe(struct auxiliary_device *adev,
> >> +				     const struct auxiliary_device_id *id)
> >> +{
> >> +	struct mlxsw_linecard_bdev *linecard_bdev =
> >> +			container_of(adev, struct mlxsw_linecard_bdev, adev);
> >> +	struct mlxsw_linecard_dev *linecard_dev;
> >> +	struct devlink *devlink;
> >> +
> >> +	devlink = devlink_alloc(&mlxsw_linecard_dev_devlink_ops,
> >> +				sizeof(*linecard_dev), &adev->dev);
> >> +	if (!devlink)
> >> +		return -ENOMEM;
> >> +	linecard_dev = devlink_priv(devlink);
> >> +	linecard_dev->linecard = linecard_bdev->linecard;
> >> +	linecard_bdev->linecard_dev = linecard_dev;
> >> +
> >> +	devlink_register(devlink);
> >> +	return 0;
> >> +}
> >
> >[...]
> >
> >> @@ -252,6 +253,14 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
> >>  	linecard->provisioned = true;
> >>  	linecard->hw_revision = hw_revision;
> >>  	linecard->ini_version = ini_version;
> >> +
> >> +	err = mlxsw_linecard_bdev_add(linecard);
> >
> >If a line card is already provisioned and we are reloading the primary
> >devlink instance, isn't this going to deadlock on the global (not
> >per-instance) devlink mutex? It is held throughout the reload operation
> >and also taken in devlink_register()
> >
> >My understanding of the auxiliary bus model is that after adding a
> >device to the bus via auxiliary_device_add(), the probe() function of
> >the auxiliary driver will be called. In our case, this function acquires
> >the global devlink mutex in devlink_register().
> 
> No, the line card auxdev is supposed to be removed during
> linecard_fini(). This, I forgot to add, will do in v2.

mlxsw_linecard_fini() is called as part of reload with the global
devlink mutex held. The removal of the auxdev should prompt the
unregistration of its devlink instance which also takes this mutex. If
this doesn't deadlock, then I'm probably missing something.

Can you test reload with lockdep when line cards are already
provisioned/active?

> 
> 
> >
> >> +	if (err) {
> >> +		linecard->provisioned = false;
> >> +		mlxsw_linecard_provision_fail(linecard);
> >> +		return err;
> >> +	}
> >> +
> >>  	devlink_linecard_provision_set(linecard->devlink_linecard, type);
> >>  	return 0;
> >>  }