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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jun 2022 18:39:59 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Ido Schimmel <idosch@...dia.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        petrm@...dia.com, pabeni@...hat.com, edumazet@...gle.com,
        mlxsw@...dia.com
Subject: Re: [patch net-next 02/11] mlxsw: core_linecards: Introduce per line
 card auxiliary device

Thu, Jun 16, 2022 at 09:11:56AM CEST, idosch@...dia.com wrote:
>On Wed, Jun 15, 2022 at 07:37:15PM +0200, Jiri Pirko wrote:
>> Wed, Jun 15, 2022 at 04:52:13PM CEST, idosch@...dia.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.

You don't miss anything, it really does. Need to remove devlink_mutex
first.


>
>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;
>> >>  }