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:   Wed, 15 Jun 2022 19:37:15 +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

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.


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