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]

Message-ID: <YrlrsXJXcwpmY0n2@nanopsycho>
Date:   Mon, 27 Jun 2022 10:34:57 +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 07:41:52PM CEST, idosch@...dia.com wrote:
>On Thu, Jun 16, 2022 at 06:39:59PM +0200, Jiri Pirko wrote:
>> 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 please send it separately? Will probably need thorough review
>and testing...

Sure.


>
>The comment above devlink_mutex is: "An overall lock guarding every
>operation coming from userspace. It also guards devlink devices list and
>it is taken when driver registers/unregisters it.", but devlink does not
>have "parallel_ops" enabled, so maybe it's enough to only use this lock
>to protect the devlink devices list?

I'm preparing a patchset that will answer all your questions, lets move
the discussion there.


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