Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On Wed, Feb 22, 2017 at 09:47:53AM -0700, Jens Axboe wrote: > I see, I found it now. Guys, let's get this process streamlined a bit > more. This whole thing has been a flurry of patches and patchseries, > posted by either you or Jon. Previous patch series was 1-4 patches > posted by you, and then patch #4 is replaced by a single patch from Jon, > posted outside of that thread. Honestly, I feel like this should have > been pushed to 4.12 instead, it clearly wasn't ready before the merge > window. Sorry, I'll run interference to help ensure future patch series are complete and coherent in one thread.
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On 02/22/2017 09:13 AM, Scott Bauer wrote: > On Wed, Feb 22, 2017 at 09:10:31AM -0700, Jens Axboe wrote: >> On 02/22/2017 12:13 AM, Christoph Hellwig wrote: + if (!lock_held) + mutex_lock(&dev->dev_lock); >>> >>> No conditional locking, please. I guess I causesd this by asking you >>> to remove __opal_lock_unlock, but it seems we'd either need to keep it >>> in the end. >>> >>> Except for that the series looks fine to me. >>> >>> Jens: given that 1-3 are the important fixes how about you pick those >>> up ASAP? They all also had my Reviewed-by for previous postings. >> >> I picked up 1-3, and re-added your reviewed by. #4 should be sorted >> before -rc1, though. >> > > #4 Is good to go as well. It was resent this morning under > [PATCH] block/sed: Embed function data into the function sequence > And contains the changes Christoph requested, I'll re-add my sign-off. > Once that gets In I can rebase mine and get them out today too. I see, I found it now. Guys, let's get this process streamlined a bit more. This whole thing has been a flurry of patches and patchseries, posted by either you or Jon. Previous patch series was 1-4 patches posted by you, and then patch #4 is replaced by a single patch from Jon, posted outside of that thread. Honestly, I feel like this should have been pushed to 4.12 instead, it clearly wasn't ready before the merge window. -- Jens Axboe
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On Wed, Feb 22, 2017 at 09:10:31AM -0700, Jens Axboe wrote: > On 02/22/2017 12:13 AM, Christoph Hellwig wrote: > >> + if (!lock_held) > >> + mutex_lock(&dev->dev_lock); > > > > No conditional locking, please. I guess I causesd this by asking you > > to remove __opal_lock_unlock, but it seems we'd either need to keep it > > in the end. > > > > Except for that the series looks fine to me. > > > > Jens: given that 1-3 are the important fixes how about you pick those > > up ASAP? They all also had my Reviewed-by for previous postings. > > I picked up 1-3, and re-added your reviewed by. #4 should be sorted > before -rc1, though. > #4 Is good to go as well. It was resent this morning under [PATCH] block/sed: Embed function data into the function sequence And contains the changes Christoph requested, I'll re-add my sign-off. Once that gets In I can rebase mine and get them out today too.
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On 02/22/2017 12:13 AM, Christoph Hellwig wrote: >> +if (!lock_held) >> +mutex_lock(&dev->dev_lock); > > No conditional locking, please. I guess I causesd this by asking you > to remove __opal_lock_unlock, but it seems we'd either need to keep it > in the end. > > Except for that the series looks fine to me. > > Jens: given that 1-3 are the important fixes how about you pick those > up ASAP? They all also had my Reviewed-by for previous postings. I picked up 1-3, and re-added your reviewed by. #4 should be sorted before -rc1, though. -- Jens Axboe
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On 02/22/2017 12:13 AM, Christoph Hellwig wrote: >> +if (!lock_held) >> +mutex_lock(&dev->dev_lock); > > No conditional locking, please. I guess I causesd this by asking you > to remove __opal_lock_unlock, but it seems we'd either need to keep it > in the end. > > Except for that the series looks fine to me. > > Jens: given that 1-3 are the important fixes how about you pick those > up ASAP? They all also had my Reviewed-by for previous postings. > Thanks I'll respin just 4/4 shortly with __opal_lock_unlock
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
> + if (!lock_held) > + mutex_lock(&dev->dev_lock); No conditional locking, please. I guess I causesd this by asking you to remove __opal_lock_unlock, but it seems we'd either need to keep it in the end. Except for that the series looks fine to me. Jens: given that 1-3 are the important fixes how about you pick those up ASAP? They all also had my Reviewed-by for previous postings.
Re: [PATCHv4 4/4] block/sed: Embed function data into the function sequence
On Tue, Feb 21, 2017 at 11:59:16AM -0700, Jon Derrick wrote: > By embedding the function data with the function sequence, we can > eliminate the external function data and state variable code. It also > made obvious some other small cleanups. > > Signed-off-by: Jon Derrick Reviewed-by: Scott Bauer Also Christoph had reviewed-by on 1-3 on friday, I dont think we need a respin, but wanted to point that out since his tag isnt on 1-3 v4: https://marc.info/?l=linux-block&m=148725565212351&w=2 https://marc.info/?l=linux-block&m=148725603312475&w=2 https://marc.info/?l=linuxppc-embedded&m=148725612212503&w=2
[PATCHv4 4/4] block/sed: Embed function data into the function sequence
By embedding the function data with the function sequence, we can
eliminate the external function data and state variable code. It also
made obvious some other small cleanups.
Signed-off-by: Jon Derrick
---
block/sed-opal.c | 428 ++-
1 file changed, 167 insertions(+), 261 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 4fc4d7b..bd8605b 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -34,7 +34,11 @@
#define IO_BUFFER_LENGTH 2048
#define MAX_TOKS 64
-typedef int (*opal_step)(struct opal_dev *dev);
+struct opal_step {
+ int (*fn)(struct opal_dev *dev, void *data);
+ void *data;
+};
+typedef int (cont_fn)(struct opal_dev *dev);
enum opal_atom_width {
OPAL_WIDTH_TINY,
@@ -80,9 +84,7 @@ struct opal_dev {
void *data;
sec_send_recv *send_recv;
- const opal_step *funcs;
- void **func_data;
- int state;
+ const struct opal_step *steps;
struct mutex dev_lock;
u16 comid;
u32 hsn;
@@ -213,8 +215,6 @@ struct opal_dev {
{ 0x00, 0x00, 0x00, 0x06, 0x00, 0x00, 0x08, 0x03 },
};
-typedef int (cont_fn)(struct opal_dev *dev);
-
static int end_opal_session_error(struct opal_dev *dev);
struct opal_suspend_data {
@@ -375,18 +375,18 @@ static void check_geometry(struct opal_dev *dev, const
void *data)
static int next(struct opal_dev *dev)
{
- opal_step func;
- int error = 0;
+ const struct opal_step *step;
+ int state = 0, error = 0;
do {
- func = dev->funcs[dev->state];
- if (!func)
+ step = &dev->steps[state];
+ if (!step->fn)
break;
- error = func(dev);
+ error = step->fn(dev, step->data);
if (error) {
pr_err("Error on step function: %d with error %d: %s\n",
- dev->state, error,
+ state, error,
opal_error_to_human(error));
/* For each OPAL command we do a discovery0 then we
@@ -396,10 +396,10 @@ static int next(struct opal_dev *dev)
* session. Therefore we shouldn't attempt to terminate
* a session, as one has not yet been created.
*/
- if (dev->state > 1)
+ if (state > 1)
return end_opal_session_error(dev);
}
- dev->state++;
+ state++;
} while (!error);
return error;
@@ -483,7 +483,7 @@ static int opal_discovery0_end(struct opal_dev *dev)
return 0;
}
-static int opal_discovery0(struct opal_dev *dev)
+static int opal_discovery0(struct opal_dev *dev, void *data)
{
int ret;
@@ -1018,7 +1018,7 @@ static int finalize_and_send(struct opal_dev *dev,
cont_fn cont)
return opal_send_recv(dev, cont);
}
-static int gen_key(struct opal_dev *dev)
+static int gen_key(struct opal_dev *dev, void *data)
{
const u8 *method;
u8 uid[OPAL_UID_LENGTH];
@@ -1072,15 +1072,14 @@ static int get_active_key_cont(struct opal_dev *dev)
return 0;
}
-static int get_active_key(struct opal_dev *dev)
+static int get_active_key(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
int err = 0;
- u8 *lr;
+ u8 *lr = data;
clear_opal_cmd(dev);
set_comid(dev, dev->comid);
- lr = dev->func_data[dev->state];
err = build_locking_range(uid, sizeof(uid), *lr);
if (err)
@@ -1163,17 +1162,16 @@ static inline int enable_global_lr(struct opal_dev
*dev, u8 *uid,
return err;
}
-static int setup_locking_range(struct opal_dev *dev)
+static int setup_locking_range(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
- struct opal_user_lr_setup *setup;
+ struct opal_user_lr_setup *setup = data;
u8 lr;
int err = 0;
clear_opal_cmd(dev);
set_comid(dev, dev->comid);
- setup = dev->func_data[dev->state];
lr = setup->session.opal_key.lr;
err = build_locking_range(uid, sizeof(uid), lr);
if (err)
@@ -1286,20 +1284,19 @@ static int start_generic_opal_session(struct opal_dev
*dev,
return finalize_and_send(dev, start_opal_session_cont);
}
-static int start_anybodyASP_opal_session(struct opal_dev *dev)
+static int start_anybodyASP_opal_session(struct opal_dev *dev, void *data)
{
return start_generic_opal_session(dev, OPAL_ANYBODY_UID,
OPAL_ADMINSP_UID, NULL, 0);
}
-static int start_SIDASP_opal_session(struct opal_dev *dev)
+static int start_SIDASP_opal_session(struct opal_dev *dev, void *data)
{
int ret;
const u8 *key = dev->prev_data;
- struct opal_key *okey;
