Re: [PATCHv2 4/5] block/sed: Embed function data into the function sequence

[Christoph Hellwig <hch@lst.de>]

Hi Jon,

I think this is a great cleanup!

A few nitpicky comments below:

> -typedef int (*opal_step)(struct opal_dev *dev);
> +typedef struct opal_step {
> +	int (*fn)(struct opal_dev *dev, void *data);
> +	void *data;
> +} opal_step;

no typedefs for structure types, please.

> +	opal_step *funcs;

maybe calls this member steps?

> +static int set_mbr_done(struct opal_dev *dev, void *data)
>  {
> -	u8 mbr_done_tf = *(u8 *)dev->func_data[dev->state];
> +	u8 mbr_done_tf = *(u8 *)data;

No need for casts when going from void * to any pointer type.  There are
a couple more instance below where the cast should be removed as well.

> +static int __opal_lock_unlock(struct opal_dev *dev,
> +			      struct opal_lock_unlock *lk_unlk)
>  {
> +	opal_step _unlock_funcs[] = {
> +		{ opal_discovery0, },
> +		{ start_auth_opal_session, &lk_unlk->session },
> +		{ NULL, lk_unlk },
> +		{ end_opal_session, },
> +		{ NULL, }
>  	};
>  
> +	if (lk_unlk->session.sum)
> +		_unlock_funcs[2].fn = lock_unlock_locking_range_sum;
> +	else
> +		_unlock_funcs[2].fn = lock_unlock_locking_range;
>  
>  	dev->funcs = _unlock_funcs;

I would suggest to just have two different arrays, and merge
__opal_lock_unlock into opal_lock_unlock.  E.g. something like:

static int opal_lock_unlock(struct opal_dev *dev,
		struct opal_lock_unlock *lk_unlk)
{
	const struct opal_step unlock_funcs[] = {
		{ opal_discovery0, },
		{ start_auth_opal_session, &lk_unlk->session },
		{ lock_unlock_locking_range, lk_unlk },
		{ end_opal_session, },
		{ NULL, }
	};
	const struct opal_step unlock_sun_funcs[] = {
		{ opal_discovery0, },
		{ start_auth_opal_session, &lk_unlk->session },
		{ lock_unlock_locking_range_sum, lk_unlk },
		{ end_opal_session, },
		{ NULL, }
	};
	int ret;

	if (lk_unlk->session.who < OPAL_ADMIN1 ||
	    lk_unlk->session.who > OPAL_USER9)
		return -EINVAL;

	mutex_lock(&dev->dev_lock);
	setup_opal_dev(dev, lk_unlk->session.sum ?
			unlock_sum_funcs : unlock_funcs);
	ret = next(dev);
	mutex_unlock(&dev->dev_lock);
	return ret;
}

and yes, I noticed that all the opal_step structures really should
be marked const, especially given that they contain function pointers
and are potential exploit targets.  Please apply that everywhere.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme