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.
