Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
Artem, > we have this patch-set and it fixes megaraid regression in v4.16. Do > you plan to mege it to v4.16-rcX? I am worried - there seem to be no > sight that this is going to me merged. They are not in the linux-next. I merged them into scsi-fixes last week. It happens push a combined fixes+queue to linux-next to get more zeroday coverage. However, most of the time linux-next is one 4.x+1 material only. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] block, bfq: keep peak_rate estimation within range 1..2^32-1
> Il giorno 19 mar 2018, alle ore 14:28, Konstantin Khlebnikov
> ha scritto:
>
> On 19.03.2018 09:03, Paolo Valente wrote:
>>> Il giorno 05 mar 2018, alle ore 04:48, Konstantin Khlebnikov
>>> ha scritto:
>>>
>>> Rate should never overflow or become zero because it is used as divider.
>>> This patch accumulates it with saturation.
>>>
>>> Signed-off-by: Konstantin Khlebnikov
>>> ---
>>> block/bfq-iosched.c |8 +---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index aeca22d91101..a236c8d541b5 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -2546,7 +2546,8 @@ static void bfq_reset_rate_computation(struct
>>> bfq_data *bfqd,
>>>
>>> static void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq)
>>> {
>>> - u32 rate, weight, divisor;
>>> + u32 weight, divisor;
>>> + u64 rate;
>>>
>>> /*
>>> * For the convergence property to hold (see comments on
>>> @@ -2634,9 +2635,10 @@ static void bfq_update_rate_reset(struct bfq_data
>>> *bfqd, struct request *rq)
>>> */
>>> bfqd->peak_rate *= divisor-1;
>>> bfqd->peak_rate /= divisor;
>>> - rate /= divisor; /* smoothing constant alpha = 1/divisor */
>>> + do_div(rate, divisor); /* smoothing constant alpha = 1/divisor */
>>>
>>> - bfqd->peak_rate += rate;
>>> + /* rate should never overlow or become zero */
>> It is bfqd->peak_rate that is used as a divider, and bfqd->peak_rate doesn't
>> risk to be zero even if the variable 'rate' is zero here.
>> So I guess the reason why you consider the possibility that bfqd->peak_rate
>> becomes zero is because of an overflow when summing 'rate'. But, according
>> to my calculations, this should be impossible with devices with sensible
>> speeds.
>> These are the reasons why I decided I could make it with a 32-bit variable,
>> without any additional clamping. Did I make any mistake in my evaluation?
>
> According to Murphy's law this is inevitable..
>
Yep. Actually Murphy has been even clement this time, by making the
failure occur to a kernel expert :)
> I've seen couple division by zero crashes in bfq_wr_duration.
> Unfortunately logs weren't recorded.
>
>> Anyway, even if I made some mistake about the maximum possible value of the
>> device rate, and the latter may be too high for bfqd->peak_rate to contain
>> it, then I guess the right solution would not be to clamp the actual rate to
>> U32_MAX, but to move bfqd->peak_rate to 64 bits. Or am I missing something
>> else?
> >>> + bfqd->peak_rate = clamp_t(u64, rate + bfqd->peak_rate, 1, U32_MAX);
>
> 32-bit should be enough and better for division.
> My patch makes sure it never overflows/underflows.
> That's cheaper than full 64-bit/64-bit division.
> Anyway 64-bit speed could overflow too. =)
>
I see your point. Still, if the mistake is not in sizing, then you
bumped into some odd bug. In this respect, I don't like much the idea
of sweeping the dust under the carpet. So, let me ask you for a
little bit more help. With your patch applied, and thus with no risk
of crashes, what about adding, right before your clamp_t, something
like:
if (!bfqd->peak_rate)
pr_crit(peak_rate>);
Once the failure shows up (Murphy permitting), we might have hints to
the bug causing it.
Apart from that, I have no problem with patches that make bfq more
robust, even in a sort of black-box way.
Thanks a lot,
Paolo
>
>>> update_thr_responsiveness_params(bfqd);
>>>
>>> reset_computation:
Re: [PATCH 11/16] treewide: simplify Kconfig dependencies for removed archs
On 14/03/2018 at 15:43:46 +0100, Arnd Bergmann wrote:
> A lot of Kconfig symbols have architecture specific dependencies.
> In those cases that depend on architectures we have already removed,
> they can be omitted.
>
> Signed-off-by: Arnd Bergmann
> ---
> block/bounce.c | 2 +-
> drivers/ide/Kconfig | 2 +-
> drivers/ide/ide-generic.c| 12 +---
> drivers/input/joystick/analog.c | 2 +-
> drivers/isdn/hisax/Kconfig | 10 +-
> drivers/net/ethernet/davicom/Kconfig | 2 +-
> drivers/net/ethernet/smsc/Kconfig| 6 +++---
> drivers/net/wireless/cisco/Kconfig | 2 +-
> drivers/pwm/Kconfig | 2 +-
> drivers/rtc/Kconfig | 2 +-
Acked-by: Alexandre Belloni
> drivers/spi/Kconfig | 4 ++--
> drivers/usb/musb/Kconfig | 2 +-
> drivers/video/console/Kconfig| 3 +--
> drivers/watchdog/Kconfig | 6 --
> drivers/watchdog/Makefile| 6 --
> fs/Kconfig.binfmt| 5 ++---
> fs/minix/Kconfig | 2 +-
> include/linux/ide.h | 7 +--
> init/Kconfig | 5 ++---
> lib/Kconfig.debug| 13 +
> lib/test_user_copy.c | 2 --
> mm/Kconfig | 7 ---
> mm/percpu.c | 4
> 23 files changed, 31 insertions(+), 77 deletions(-)
>
> diff --git a/block/bounce.c b/block/bounce.c
> index 6a3e68292273..dd0b93f2a871 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -31,7 +31,7 @@
> static struct bio_set *bounce_bio_set, *bounce_bio_split;
> static mempool_t *page_pool, *isa_page_pool;
>
> -#if defined(CONFIG_HIGHMEM) || defined(CONFIG_NEED_BOUNCE_POOL)
> +#if defined(CONFIG_HIGHMEM)
> static __init int init_emergency_pool(void)
> {
> #if defined(CONFIG_HIGHMEM) && !defined(CONFIG_MEMORY_HOTPLUG)
> diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> index cf1fb3fb5d26..901b8833847f 100644
> --- a/drivers/ide/Kconfig
> +++ b/drivers/ide/Kconfig
> @@ -200,7 +200,7 @@ comment "IDE chipset support/bugfixes"
>
> config IDE_GENERIC
> tristate "generic/default IDE chipset support"
> - depends on ALPHA || X86 || IA64 || M32R || MIPS || ARCH_RPC
> + depends on ALPHA || X86 || IA64 || MIPS || ARCH_RPC
> default ARM && ARCH_RPC
> help
> This is the generic IDE driver. This driver attaches to the
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 54d7c4685d23..80c0d69b83ac 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
> @@ -13,13 +13,10 @@
> #include
> #include
>
> -/* FIXME: convert arm and m32r to use ide_platform host driver */
> +/* FIXME: convert arm to use ide_platform host driver */
> #ifdef CONFIG_ARM
> #include
> #endif
> -#ifdef CONFIG_M32R
> -#include
> -#endif
>
> #define DRV_NAME "ide_generic"
>
> @@ -35,13 +32,6 @@ static const struct ide_port_info ide_generic_port_info = {
> #ifdef CONFIG_ARM
> static const u16 legacy_bases[] = { 0x1f0 };
> static const int legacy_irqs[] = { IRQ_HARDDISK };
> -#elif defined(CONFIG_PLAT_M32700UT) || defined(CONFIG_PLAT_MAPPI2) || \
> - defined(CONFIG_PLAT_OPSPUT)
> -static const u16 legacy_bases[] = { 0x1f0 };
> -static const int legacy_irqs[] = { PLD_IRQ_CFIREQ };
> -#elif defined(CONFIG_PLAT_MAPPI3)
> -static const u16 legacy_bases[] = { 0x1f0, 0x170 };
> -static const int legacy_irqs[] = { PLD_IRQ_CFIREQ, PLD_IRQ_IDEIREQ };
> #elif defined(CONFIG_ALPHA)
> static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168 };
> static const int legacy_irqs[] = { 14, 15, 11, 10 };
> diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
> index be1b4921f22a..eefac7978f93 100644
> --- a/drivers/input/joystick/analog.c
> +++ b/drivers/input/joystick/analog.c
> @@ -163,7 +163,7 @@ static unsigned int get_time_pit(void)
> #define GET_TIME(x) do { x = (unsigned int)rdtsc(); } while (0)
> #define DELTA(x,y) ((y)-(x))
> #define TIME_NAME"TSC"
> -#elif defined(__alpha__) || defined(CONFIG_ARM) || defined(CONFIG_ARM64) ||
> defined(CONFIG_RISCV) || defined(CONFIG_TILE)
> +#elif defined(__alpha__) || defined(CONFIG_ARM) || defined(CONFIG_ARM64) ||
> defined(CONFIG_RISCV)
> #define GET_TIME(x) do { x = get_cycles(); } while (0)
> #define DELTA(x,y) ((y)-(x))
> #define TIME_NAME"get_cycles"
> diff --git a/drivers/isdn/hisax/Kconfig b/drivers/isdn/hisax/Kconfig
> index eb83d94ab4fe..38cfc8baae19 100644
> --- a/drivers/isdn/hisax/Kconfig
> +++ b/drivers/isdn/hisax/Kconfig
> @@ -109,7 +109,7 @@ config HISAX_16_3
>
> config HISAX_TELESPCI
> bool "Teles PCI"
> - depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS
> && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
> + depends on PCI && (BROKEN || !(
Re: [PATCH v2 06/11] block: sed-opal: split generation of bytestring header and content
On Mon, Mar 19, 2018 at 08:59:45PM +0100, Christoph Hellwig wrote:
> > +static u8 *add_bytestring_header(int *err, struct opal_dev *cmd, size_t
> > len)
> > {
> > size_t header_len = 1;
> > bool is_short_atom = true;
> > -
> > - if (*err)
> > - return;
> > + char *start;
>
> Shouldn't this also return early if we have a pending error?
It will short circuit and bail out via can_add failing. So even though
you have to go dig to see if the following functions handle the erorr
I think it's slightly cleaner to have a single if (*err) in the deeper
functions.
This lest the error back out the call chain instead of having multiple if (*err)
checks earlier in the call chains.
Re: [PATCH v2 11/11] block: sed-opal: check size of shadow mbr
On Mon, Mar 19, 2018 at 07:36:53PM +0100, Jonas Rabenstein wrote: > Check whether the shadow mbr does fit in the provided space on the > target. Also a proper firmware should handle this case and return an > error we may prevent problems or even damage with crappy firmwares. > > Signed-off-by: Jonas Rabenstein > > diff --git a/block/sed-opal.c b/block/sed-opal.c > index 51f8034edbf7..9c73bd24c55f 100644 > --- a/block/sed-opal.c > +++ b/block/sed-opal.c > @@ -1545,6 +1545,20 @@ static int write_shadow_mbr(struct opal_dev *dev, void > *data) > u64 len; > int err = 0; > > + /* do we fit in the available shadow mbr space? */ > + err = generic_get_table_info(dev, OPAL_MBR, OPAL_TABLE_ROWS); And here it gets used. So this should be merged with the previous patch.
Re: [PATCH v2 10/11] block: sed-opal: get metadata about opal-sed tables
On Mon, Mar 19, 2018 at 07:36:52PM +0100, Jonas Rabenstein wrote: > Every opal-sed table is described in the OPAL_TABLE_TABLE. Provide a > function to get desired metadata information out of that table. Your new function doesn't seem to be used at all.
Re: [PATCH v2 07/11] block: sed-opal: add ioctl for done-mark of shadow mbr
Looks fine: Reviewed-by: Christoph Hellwig
Re: [PATCH v2 06/11] block: sed-opal: split generation of bytestring header and content
> +static u8 *add_bytestring_header(int *err, struct opal_dev *cmd, size_t len)
> {
> size_t header_len = 1;
> bool is_short_atom = true;
> -
> - if (*err)
> - return;
> + char *start;
Shouldn't this also return early if we have a pending error?
Except for that this looks fine:
Reviewed-by: Christoph Hellwig
Re: [PATCH v2 00/11] block: sed-opal support write to shadow mbr
On Mon, Mar 19, 2018 at 08:53:35PM +0100, Christoph Hellwig wrote: > On Mon, Mar 19, 2018 at 07:36:42PM +0100, Jonas Rabenstein wrote: > > Hi, > > I was advised to resend the patchset as a v2 where all the patches are > > in a flat hierarchy. So here is a complete set which hopefully pleases > > all requirements. > > As the previous fixes have by now all landed into linux-next, no > > additional patches are required for testing. > > Btw, what userspace do you use for this? The only one I know so far > is Scotts sed-opal-temp, which really should grow a more permanent > name / location. He has a forked verison here: https://github.com/ghostav/sed-opal-temp He sent out a v1, obviously, but I had him clean up/resubmit and CC you. So you could chime in on the new ioctl. v1: https://marc.info/?l=linux-block&m=152094656909515&w=4 As for the moving of sed-opal-temp I am more than willing to place this else where. I just don't have a location other than github to place it. If people have suggestions on where to store it that *isnt* on my personal github I will do that. And give access to the necessary maintainers.
Re: [PATCH v2 05/11] block: sed-opal: print failed function address
On Mon, Mar 19, 2018 at 07:36:47PM +0100, Jonas Rabenstein wrote: > Add function address (and if available its symbol) to the message if a > step function fails. Looks good: Reviewed-by: Christoph Hellwig
Re: [PATCH v2 04/11] block: sed-opal: unify error handling of responses
On Mon, Mar 19, 2018 at 07:36:46PM +0100, Jonas Rabenstein wrote:
> Also response_get_token had already been in place, its functionality had
> been duplicated within response_get_{u64,bytestring} with the same error
> handling. Unify the handling by reusing response_get_token within the
> other functions.
Should probably be one patch for each of the two separate changes.
Except for that this looks fine to me:
Reviewed-by: Christoph Hellwig
Re: [PATCH v2 03/11] block: sed-opal: unify cmd start and finalize
On Mon, Mar 19, 2018 at 07:36:45PM +0100, Jonas Rabenstein wrote:
> Every step starts with resetting the cmd buffer as well as the comid and
> constructs the appropriate OPAL_CALL command. Consequently, those
> actions may be combined into one generic function. On should take care,
> that the opening and closing tokens for the argument list are already
> emitted by those functions and thus must not be additionally added.
>
> Signed-off-by: Jonas Rabenstein
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 771b4cfff95c..efe5d2a7f3dc 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -656,6 +656,9 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn,
> u32 tsn)
> struct opal_header *hdr;
> int err = 0;
>
> + /* close the parameter list opened from start_opal_cmd */
> + add_token_u8(&err, cmd, OPAL_ENDLIST);
> +
> add_token_u8(&err, cmd, OPAL_ENDOFDATA);
> add_token_u8(&err, cmd, OPAL_STARTLIST);
> add_token_u8(&err, cmd, 0);
I think this should be a separate patch, independent of the newly added
start_opal_cmd.
> @@ -998,6 +1001,26 @@ static void clear_opal_cmd(struct opal_dev *dev)
> memset(dev->cmd, 0, IO_BUFFER_LENGTH);
> }
>
> +static int start_opal_cmd(struct opal_dev *dev, const u8 *uid, const u8
> *method)
> +{
> + int err = 0;
start_opal_cmd and cmd_finalize don't really seem to match in terms
of naming. I don't really care either way, but a little consistency
would be nice.
> + /* every method call is followed by its parameters enclosed within
> + * OPAL_STARTLIST and OPAL_ENDLIST tokens. We automatically open the
> + * parameter list here and close it later in cmd_finalize
> + */
Normal Linux comment style would be:
/*
* Every method call is followed by its parameters enclosed within
* OPAL_STARTLIST and OPAL_ENDLIST tokens. We automatically open the
* parameter list here and close it later in cmd_finalize.
*/
Re: [PATCH v2 02/11] block: sed-opal: unify space check in add_token_*
On Mon, Mar 19, 2018 at 07:36:44PM +0100, Jonas Rabenstein wrote: > All add_token_* functions have a common set of conditions that have to > be checked. Use a common function for those checks in order to avoid > different behaviour as well as code duplication. > > Signed-off-by: Jonas Rabenstein Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH v2 00/11] block: sed-opal support write to shadow mbr
On Mon, Mar 19, 2018 at 07:36:42PM +0100, Jonas Rabenstein wrote: > Hi, > I was advised to resend the patchset as a v2 where all the patches are > in a flat hierarchy. So here is a complete set which hopefully pleases > all requirements. > As the previous fixes have by now all landed into linux-next, no > additional patches are required for testing. Btw, what userspace do you use for this? The only one I know so far is Scotts sed-opal-temp, which really should grow a more permanent name / location.
Re: [PATCH v2 01/11] block: sed-opal: use correct macro for method length
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH v2 08/11] block: sed-opal: ioctl for writing to shadow mbr
On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > Allow modification of the shadow mbr. If the shadow mbr is not marked as > done, this data will be presented read only as the device content. Only > after marking the shadow mbr as done and unlocking a locking range the > actual content is accessible. I hate doing this as an ioctls. Can we make this a sysfs binary file so that people can use dd or cat to write the shadow mbr?
Re: [PATCH] block: Change a rcu_read_{lock,unlock}_sched() pair into rcu_read_{lock,unlock}()
On Mon, Mar 19, 2018 at 11:01:00AM -0700, Bart Van Assche wrote:
> Since neither the RCU-protected code in blk_queue_enter() nor
> blk_queue_usage_counter_release() sleeps, regular RCU protection
> is sufficient. Note: scsi_device_quiesce() does not have to be
> modified since it already uses synchronize_rcu().
>
> Reported-by: Tejun Heo
> Fixes: 3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work
> reliably")
> Signed-off-by: Bart Van Assche
> Cc: Hannes Reinecke
> Cc: Ming Lei
> Cc: Christoph Hellwig
> Cc: Johannes Thumshirn
> Cc: Tejun Heo
> Cc: Oleksandr Natalenko
> Cc: Martin Steigerwald
> Cc: sta...@vger.kernel.org # v4.15
Acked-by: Tejun Heo
It'd be great to add a comment to clarify the pairing.
Thanks.
--
tejun
Re: [PATCH v2] block: Change a rcu_read_{lock,unlock}_sched() pair into rcu_read_{lock,unlock}()
On 3/19/18 12:46 PM, Bart Van Assche wrote: > scsi_device_quiesce() uses synchronize_rcu() to guarantee that the > effect of blk_set_preempt_only() will be visible for percpu_ref_tryget() > calls that occur after the queue unfreeze by using the approach > explained in https://lwn.net/Articles/573497/. The rcu read lock and > unlock calls in blk_queue_enter() form a pair with the synchronize_rcu() > call in scsi_device_quiesce(). Both scsi_device_quiesce() and > blk_queue_enter() must either use regular RCU or RCU-sched. > Since neither the RCU-protected code in blk_queue_enter() nor > blk_queue_usage_counter_release() sleeps, regular RCU protection > is sufficient. Note: scsi_device_quiesce() does not have to be > modified since it already uses synchronize_rcu(). Applied, thanks. -- Jens Axboe
[PATCH v2] block: Change a rcu_read_{lock,unlock}_sched() pair into rcu_read_{lock,unlock}()
scsi_device_quiesce() uses synchronize_rcu() to guarantee that the
effect of blk_set_preempt_only() will be visible for percpu_ref_tryget()
calls that occur after the queue unfreeze by using the approach
explained in https://lwn.net/Articles/573497/. The rcu read lock and
unlock calls in blk_queue_enter() form a pair with the synchronize_rcu()
call in scsi_device_quiesce(). Both scsi_device_quiesce() and
blk_queue_enter() must either use regular RCU or RCU-sched.
Since neither the RCU-protected code in blk_queue_enter() nor
blk_queue_usage_counter_release() sleeps, regular RCU protection
is sufficient. Note: scsi_device_quiesce() does not have to be
modified since it already uses synchronize_rcu().
Reported-by: Tejun Heo
Fixes: 3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work reliably")
Signed-off-by: Bart Van Assche
Acked-by: Tejun Heo
Cc: Tejun Heo
Cc: Hannes Reinecke
Cc: Ming Lei
Cc: Christoph Hellwig
Cc: Johannes Thumshirn
Cc: Oleksandr Natalenko
Cc: Martin Steigerwald
Cc: sta...@vger.kernel.org # v4.15
---
Changes between v1 and v2:
- Explained in the interaction between scsi_device_quiesce() and
blk_queue_enter() in the patch description.
block/blk-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 5e88c579e896..a0f675f84f86 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -917,7 +917,7 @@ int blk_queue_enter(struct request_queue *q,
blk_mq_req_flags_t flags)
bool success = false;
int ret;
- rcu_read_lock_sched();
+ rcu_read_lock();
if (percpu_ref_tryget_live(&q->q_usage_counter)) {
/*
* The code that sets the PREEMPT_ONLY flag is
@@ -930,7 +930,7 @@ int blk_queue_enter(struct request_queue *q,
blk_mq_req_flags_t flags)
percpu_ref_put(&q->q_usage_counter);
}
}
- rcu_read_unlock_sched();
+ rcu_read_unlock();
if (success)
return 0;
--
2.16.2
[PATCH v2 02/11] block: sed-opal: unify space check in add_token_*
All add_token_* functions have a common set of conditions that have to
be checked. Use a common function for those checks in order to avoid
different behaviour as well as code duplication.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 5aa41744b8f1..771b4cfff95c 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -510,15 +510,24 @@ static int opal_discovery0(struct opal_dev *dev, void
*data)
return opal_discovery0_end(dev);
}
-static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
+static bool can_add(int *err, struct opal_dev *cmd, size_t len)
{
if (*err)
- return;
- if (cmd->pos >= IO_BUFFER_LENGTH - 1) {
- pr_debug("Error adding u8: end of buffer.\n");
+ return false;
+
+ if (len > IO_BUFFER_LENGTH || cmd->pos >= IO_BUFFER_LENGTH - len) {
+ pr_debug("Error adding %zu bytes: end of buffer.\n", len);
*err = -ERANGE;
- return;
+ return false;
}
+
+ return true;
+}
+
+static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
+{
+ if (!can_add(err, cmd, 1))
+ return;
cmd->cmd[cmd->pos++] = tok;
}
@@ -563,9 +572,8 @@ static void add_token_u64(int *err, struct opal_dev *cmd,
u64 number)
msb = fls64(number);
len = DIV_ROUND_UP(msb, 8);
- if (cmd->pos >= IO_BUFFER_LENGTH - len - 1) {
+ if (!can_add(err, cmd, len + 1)) {
pr_debug("Error adding u64: end of buffer.\n");
- *err = -ERANGE;
return;
}
add_short_atom_header(cmd, false, false, len);
@@ -587,9 +595,8 @@ static void add_token_bytestring(int *err, struct opal_dev
*cmd,
is_short_atom = false;
}
- if (len >= IO_BUFFER_LENGTH - cmd->pos - header_len) {
+ if (!can_add(err, cmd, header_len + len)) {
pr_debug("Error adding bytestring: end of buffer.\n");
- *err = -ERANGE;
return;
}
--
2.16.1
[PATCH v2 01/11] block: sed-opal: use correct macro for method length
Also the values of OPAL_UID_LENGTH and OPAL_METHOD_LENGTH are the same,
it is weird to use OPAL_UID_LENGTH for the definition of the methods.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 945f4b8610e0..5aa41744b8f1 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -181,7 +181,7 @@ static const u8 opaluid[][OPAL_UID_LENGTH] = {
* Derived from: TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
* Section: 6.3 Assigned UIDs
*/
-static const u8 opalmethod[][OPAL_UID_LENGTH] = {
+static const u8 opalmethod[][OPAL_METHOD_LENGTH] = {
[OPAL_PROPERTIES] =
{ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0x01 },
[OPAL_STARTSESSION] =
--
2.16.1
[PATCH v2 00/11] block: sed-opal support write to shadow mbr
Hi, I was advised to resend the patchset as a v2 where all the patches are in a flat hierarchy. So here is a complete set which hopefully pleases all requirements. As the previous fixes have by now all landed into linux-next, no additional patches are required for testing. Thanks, Jonas -- Jonas Rabenstein (11): block: sed-opal: use correct macro for method length block: sed-opal: unify space check in add_token_* block: sed-opal: unify cmd start and finalize block: sed-opal: unify error handling of responses block: sed-opal: print failed function address block: sed-opal: split generation of bytestring header and content block: sed-opal: add ioctl for done-mark of shadow mbr block: sed-opal: ioctl for writing to shadow mbr block: sed-opal: unify retrieval of table columns block: sed-opal: get metadata about opal-sed tables block: sed-opal: check size of shadow mbr block/opal_proto.h| 18 ++ block/sed-opal.c | 619 +- include/linux/sed-opal.h | 2 + include/uapi/linux/sed-opal.h | 9 + 4 files changed, 339 insertions(+), 309 deletions(-) -- 2.16.1
[PATCH v2 03/11] block: sed-opal: unify cmd start and finalize
Every step starts with resetting the cmd buffer as well as the comid and
constructs the appropriate OPAL_CALL command. Consequently, those
actions may be combined into one generic function. On should take care,
that the opening and closing tokens for the argument list are already
emitted by those functions and thus must not be additionally added.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 771b4cfff95c..efe5d2a7f3dc 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -656,6 +656,9 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32
tsn)
struct opal_header *hdr;
int err = 0;
+ /* close the parameter list opened from start_opal_cmd */
+ add_token_u8(&err, cmd, OPAL_ENDLIST);
+
add_token_u8(&err, cmd, OPAL_ENDOFDATA);
add_token_u8(&err, cmd, OPAL_STARTLIST);
add_token_u8(&err, cmd, 0);
@@ -998,6 +1001,26 @@ static void clear_opal_cmd(struct opal_dev *dev)
memset(dev->cmd, 0, IO_BUFFER_LENGTH);
}
+static int start_opal_cmd(struct opal_dev *dev, const u8 *uid, const u8
*method)
+{
+ int err = 0;
+
+ clear_opal_cmd(dev);
+ set_comid(dev, dev->comid);
+
+ add_token_u8(&err, dev, OPAL_CALL);
+ add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
+ add_token_bytestring(&err, dev, method, OPAL_METHOD_LENGTH);
+
+ /* every method call is followed by its parameters enclosed within
+* OPAL_STARTLIST and OPAL_ENDLIST tokens. We automatically open the
+* parameter list here and close it later in cmd_finalize
+*/
+ add_token_u8(&err, dev, OPAL_STARTLIST);
+
+ return err;
+}
+
static int start_opal_session_cont(struct opal_dev *dev)
{
u32 hsn, tsn;
@@ -1060,21 +1083,13 @@ static int finalize_and_send(struct opal_dev *dev,
cont_fn cont)
static int gen_key(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
- int err = 0;
-
- clear_opal_cmd(dev);
- set_comid(dev, dev->comid);
+ int err;
memcpy(uid, dev->prev_data, min(sizeof(uid), dev->prev_d_len));
kfree(dev->prev_data);
dev->prev_data = NULL;
- add_token_u8(&err, dev, OPAL_CALL);
- add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
- add_token_bytestring(&err, dev, opalmethod[OPAL_GENKEY],
-OPAL_UID_LENGTH);
- add_token_u8(&err, dev, OPAL_STARTLIST);
- add_token_u8(&err, dev, OPAL_ENDLIST);
+ err = start_opal_cmd(dev, uid, opalmethod[OPAL_GENKEY]);
if (err) {
pr_debug("Error building gen key command\n");
@@ -1112,21 +1127,14 @@ static int get_active_key_cont(struct opal_dev *dev)
static int get_active_key(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
- int err = 0;
+ int err;
u8 *lr = data;
- clear_opal_cmd(dev);
- set_comid(dev, dev->comid);
-
err = build_locking_range(uid, sizeof(uid), *lr);
if (err)
return err;
- err = 0;
- add_token_u8(&err, dev, OPAL_CALL);
- add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
- add_token_bytestring(&err, dev, opalmethod[OPAL_GET], OPAL_UID_LENGTH);
- add_token_u8(&err, dev, OPAL_STARTLIST);
+ err = start_opal_cmd(dev, uid, opalmethod[OPAL_GET]);
add_token_u8(&err, dev, OPAL_STARTLIST);
add_token_u8(&err, dev, OPAL_STARTNAME);
add_token_u8(&err, dev, 3); /* startCloumn */
@@ -1137,7 +1145,6 @@ static int get_active_key(struct opal_dev *dev, void
*data)
add_token_u8(&err, dev, 10); /* ActiveKey */
add_token_u8(&err, dev, OPAL_ENDNAME);
add_token_u8(&err, dev, OPAL_ENDLIST);
- add_token_u8(&err, dev, OPAL_ENDLIST);
if (err) {
pr_debug("Error building get active key command\n");
return err;
@@ -1150,13 +1157,10 @@ static int generic_lr_enable_disable(struct opal_dev
*dev,
u8 *uid, bool rle, bool wle,
bool rl, bool wl)
{
- int err = 0;
+ int err;
- add_token_u8(&err, dev, OPAL_CALL);
- add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
- add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
+ err = start_opal_cmd(dev, uid, opalmethod[OPAL_SET]);
- add_token_u8(&err, dev, OPAL_STARTLIST);
add_token_u8(&err, dev, OPAL_STARTNAME);
add_token_u8(&err, dev, OPAL_VALUES);
add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1183,7 +1187,6 @@ static int generic_lr_enable_disable(struct opal_dev *dev,
add_token_u8(&err, dev, OPAL_ENDLIST);
add_token_u8(&err, dev, OPAL_ENDNAME);
- add_token_u8(&err, dev, OPAL_ENDLIST);
return err;
}
@@ -1204,10 +1207,7 @@ static int setup_locking_range(struct opal_dev *dev,
void *data)
u8 uid[OPAL_UID_LENGTH];
[PATCH v2 04/11] block: sed-opal: unify error handling of responses
Also response_get_token had already been in place, its functionality had
been duplicated within response_get_{u64,bytestring} with the same error
handling. Unify the handling by reusing response_get_token within the
other functions.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index efe5d2a7f3dc..30f6e46518a6 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -697,6 +697,11 @@ static const struct opal_resp_tok *response_get_token(
{
const struct opal_resp_tok *tok;
+ if (!resp) {
+ pr_debug("Response is NULL\n");
+ return ERR_PTR(-EINVAL);
+ }
+
if (n >= resp->num) {
pr_debug("Token number doesn't exist: %d, resp: %d\n",
n, resp->num);
@@ -879,27 +884,19 @@ static size_t response_get_string(const struct
parsed_resp *resp, int n,
const char **store)
{
u8 skip;
- const struct opal_resp_tok *token;
+ const struct opal_resp_tok *tok;
*store = NULL;
- if (!resp) {
- pr_debug("Response is NULL\n");
- return 0;
- }
-
- if (n > resp->num) {
- pr_debug("Response has %d tokens. Can't access %d\n",
-resp->num, n);
+ tok = response_get_token(resp, n);
+ if (IS_ERR(tok))
return 0;
- }
- token = &resp->toks[n];
- if (token->type != OPAL_DTA_TOKENID_BYTESTRING) {
+ if (tok->type != OPAL_DTA_TOKENID_BYTESTRING) {
pr_debug("Token is not a byte string!\n");
return 0;
}
- switch (token->width) {
+ switch (tok->width) {
case OPAL_WIDTH_TINY:
case OPAL_WIDTH_SHORT:
skip = 1;
@@ -915,37 +912,29 @@ static size_t response_get_string(const struct
parsed_resp *resp, int n,
return 0;
}
- *store = token->pos + skip;
- return token->len - skip;
+ *store = tok->pos + skip;
+ return tok->len - skip;
}
static u64 response_get_u64(const struct parsed_resp *resp, int n)
{
- if (!resp) {
- pr_debug("Response is NULL\n");
- return 0;
- }
+ const struct opal_resp_tok *tok;
- if (n > resp->num) {
- pr_debug("Response has %d tokens. Can't access %d\n",
-resp->num, n);
+ tok = response_get_token(resp, n);
+ if (IS_ERR(tok))
return 0;
- }
- if (resp->toks[n].type != OPAL_DTA_TOKENID_UINT) {
- pr_debug("Token is not unsigned it: %d\n",
-resp->toks[n].type);
+ if (tok->type != OPAL_DTA_TOKENID_UINT) {
+ pr_debug("Token is not unsigned it: %d\n", tok->type);
return 0;
}
- if (!(resp->toks[n].width == OPAL_WIDTH_TINY ||
- resp->toks[n].width == OPAL_WIDTH_SHORT)) {
- pr_debug("Atom is not short or tiny: %d\n",
-resp->toks[n].width);
+ if (tok->width != OPAL_WIDTH_TINY && tok->width != OPAL_WIDTH_SHORT) {
+ pr_debug("Atom is not short or tiny: %d\n", tok->width);
return 0;
}
- return resp->toks[n].stored.u;
+ return tok->stored.u;
}
static bool response_token_matches(const struct opal_resp_tok *token, u8 match)
--
2.16.1
[PATCH v2 07/11] block: sed-opal: add ioctl for done-mark of shadow mbr
Enable users to mark the shadow mbr as done without completely
deactivating the shadow mbr feature. This may be useful on reboots,
when the power to the disk is not disconnected in between and the shadow
mbr stores the required boot files. Of course, this saves also the
(few) commands required to enable the feature if it is already enabled
and one only wants to mark the shadow mbr as done.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index fc10f81d4892..2c8baff8bf67 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1988,13 +1988,39 @@ static int opal_erase_locking_range(struct opal_dev
*dev,
static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
struct opal_mbr_data *opal_mbr)
{
+ u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
+ ? OPAL_TRUE : OPAL_FALSE;
const struct opal_step mbr_steps[] = {
{ opal_discovery0, },
{ start_admin1LSP_opal_session, &opal_mbr->key },
- { set_mbr_done, &opal_mbr->enable_disable },
+ { set_mbr_done, &token },
{ end_opal_session, },
{ start_admin1LSP_opal_session, &opal_mbr->key },
- { set_mbr_enable_disable, &opal_mbr->enable_disable },
+ { set_mbr_enable_disable, &token },
+ { end_opal_session, },
+ { NULL, }
+ };
+ int ret;
+
+ if (opal_mbr->enable_disable != OPAL_MBR_ENABLE &&
+ opal_mbr->enable_disable != OPAL_MBR_DISABLE)
+ return -EINVAL;
+
+ mutex_lock(&dev->dev_lock);
+ setup_opal_dev(dev, mbr_steps);
+ ret = next(dev);
+ mutex_unlock(&dev->dev_lock);
+ return ret;
+}
+
+static int opal_mbr_status(struct opal_dev *dev, struct opal_mbr_data
*opal_mbr)
+{
+ u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
+ ? OPAL_TRUE : OPAL_FALSE;
+ const struct opal_step mbr_steps[] = {
+ { opal_discovery0, },
+ { start_admin1LSP_opal_session, &opal_mbr->key },
+ { set_mbr_done, &token },
{ end_opal_session, },
{ NULL, }
};
@@ -2340,6 +2366,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
void __user *arg)
case IOC_OPAL_ENABLE_DISABLE_MBR:
ret = opal_enable_disable_shadow_mbr(dev, p);
break;
+ case IOC_OPAL_MBR_STATUS:
+ ret = opal_mbr_status(dev, p);
+ break;
case IOC_OPAL_ERASE_LR:
ret = opal_erase_locking_range(dev, p);
break;
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 04b124fca51e..b38dc602cae3 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -47,6 +47,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
case IOC_OPAL_ENABLE_DISABLE_MBR:
case IOC_OPAL_ERASE_LR:
case IOC_OPAL_SECURE_ERASE_LR:
+ case IOC_OPAL_MBR_STATUS:
return true;
}
return false;
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index 627624d35030..0cb9890cdc04 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -116,5 +116,6 @@ struct opal_mbr_data {
#define IOC_OPAL_ENABLE_DISABLE_MBR _IOW('p', 229, struct opal_mbr_data)
#define IOC_OPAL_ERASE_LR _IOW('p', 230, struct opal_session_info)
#define IOC_OPAL_SECURE_ERASE_LR_IOW('p', 231, struct opal_session_info)
+#define IOC_OPAL_MBR_STATUS _IOW('p', 232, struct opal_mbr_data)
#endif /* _UAPI_SED_OPAL_H */
--
2.16.1
[PATCH v2 08/11] block: sed-opal: ioctl for writing to shadow mbr
Allow modification of the shadow mbr. If the shadow mbr is not marked as
done, this data will be presented read only as the device content. Only
after marking the shadow mbr as done and unlocking a locking range the
actual content is accessible.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 2c8baff8bf67..4549fa164e98 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1492,6 +1492,54 @@ static int set_mbr_enable_disable(struct opal_dev *dev,
void *data)
return finalize_and_send(dev, parse_and_check_status);
}
+static int write_shadow_mbr(struct opal_dev *dev, void *data)
+{
+ struct opal_shadow_mbr *shadow = data;
+ const u8 __user *src;
+ u8 *dst;
+ size_t off;
+ u64 len;
+ int err = 0;
+
+ /* FIXME: this is the maximum we can use for IO_BUFFER_LENGTH=2048.
+*Instead of having a constant value, it would be nice to
+*compute the actual value depending on IO_BUFFER_LENGTH
+*/
+ len = 1950;
+
+ /* do the actual transmission(s) */
+ src = (u8 *) shadow->data;
+ for (off = 0 ; off < shadow->size; off += len) {
+ len = min(len, shadow->size - off);
+
+ pr_debug("MBR: write bytes %zu+%llu/%llu\n",
+off, len, shadow->size);
+ err = start_opal_cmd(dev, opaluid[OPAL_MBR],
+opalmethod[OPAL_SET]);
+ add_token_u8(&err, dev, OPAL_STARTNAME);
+ add_token_u8(&err, dev, OPAL_WHERE);
+ add_token_u64(&err, dev, shadow->offset + off);
+ add_token_u8(&err, dev, OPAL_ENDNAME);
+
+ add_token_u8(&err, dev, OPAL_STARTNAME);
+ add_token_u8(&err, dev, OPAL_VALUES);
+ dst = add_bytestring_header(&err, dev, len);
+ if (!dst)
+ break;
+ if (copy_from_user(dst, src + off, len))
+ err = -EFAULT;
+
+ add_token_u8(&err, dev, OPAL_ENDNAME);
+ if (err)
+ break;
+
+ err = finalize_and_send(dev, parse_and_check_status);
+ if (err)
+ break;
+ }
+ return err;
+}
+
static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid,
struct opal_dev *dev)
{
@@ -2037,6 +2085,31 @@ static int opal_mbr_status(struct opal_dev *dev, struct
opal_mbr_data *opal_mbr)
return ret;
}
+static int opal_write_shadow_mbr(struct opal_dev *dev,
+struct opal_shadow_mbr *info)
+{
+ const struct opal_step mbr_steps[] = {
+ { opal_discovery0, },
+ { start_admin1LSP_opal_session, &info->key },
+ { write_shadow_mbr, info },
+ { end_opal_session, },
+ { NULL, }
+ };
+ int ret;
+
+ if (info->size == 0)
+ return 0;
+
+ if (!access_ok(VERIFY_READ, info->data, info->size))
+ return -EINVAL;
+
+ mutex_lock(&dev->dev_lock);
+ setup_opal_dev(dev, mbr_steps);
+ ret = next(dev);
+ mutex_unlock(&dev->dev_lock);
+ return ret;
+}
+
static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
{
struct opal_suspend_data *suspend;
@@ -2369,6 +2442,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
void __user *arg)
case IOC_OPAL_MBR_STATUS:
ret = opal_mbr_status(dev, p);
break;
+ case IOC_OPAL_WRITE_SHADOW_MBR:
+ ret = opal_write_shadow_mbr(dev, p);
+ break;
case IOC_OPAL_ERASE_LR:
ret = opal_erase_locking_range(dev, p);
break;
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index b38dc602cae3..cf08cdc13cbd 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -47,6 +47,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
case IOC_OPAL_ENABLE_DISABLE_MBR:
case IOC_OPAL_ERASE_LR:
case IOC_OPAL_SECURE_ERASE_LR:
+ case IOC_OPAL_WRITE_SHADOW_MBR:
case IOC_OPAL_MBR_STATUS:
return true;
}
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index 0cb9890cdc04..8e84307f66d4 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -104,6 +104,13 @@ struct opal_mbr_data {
__u8 __align[7];
};
+struct opal_shadow_mbr {
+ struct opal_key key;
+ const __u64 data;
+ __u64 offset;
+ __u64 size;
+};
+
#define IOC_OPAL_SAVE _IOW('p', 220, struct opal_lock_unlock)
#define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock)
#define IOC_OPAL_TAKE_OWNERSHIP_IOW('p', 222, struct opal_key)
@@ -117,5 +124,6 @@ struct opal_mbr_data {
#define IOC_OPAL_ERASE_LR _IOW('p', 2
[PATCH v2 11/11] block: sed-opal: check size of shadow mbr
Check whether the shadow mbr does fit in the provided space on the
target. Also a proper firmware should handle this case and return an
error we may prevent problems or even damage with crappy firmwares.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 51f8034edbf7..9c73bd24c55f 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1545,6 +1545,20 @@ static int write_shadow_mbr(struct opal_dev *dev, void
*data)
u64 len;
int err = 0;
+ /* do we fit in the available shadow mbr space? */
+ err = generic_get_table_info(dev, OPAL_MBR, OPAL_TABLE_ROWS);
+ if (err) {
+ pr_debug("MBR: could not get shadow size\n");
+ return err;
+ }
+
+ len = response_get_u64(&dev->parsed, 4);
+ if (shadow->offset + shadow->size > len) {
+ pr_debug("MBR: does not fit in shadow (%llu vs. %llu)\n",
+shadow->offset + shadow->size, len);
+ return -ENOSPC;
+ }
+
/* FIXME: this is the maximum we can use for IO_BUFFER_LENGTH=2048.
*Instead of having a constant value, it would be nice to
*compute the actual value depending on IO_BUFFER_LENGTH
--
2.16.1
[PATCH v2 10/11] block: sed-opal: get metadata about opal-sed tables
Every opal-sed table is described in the OPAL_TABLE_TABLE. Provide a
function to get desired metadata information out of that table.
Signed-off-by: Jonas Rabenstein
diff --git a/block/opal_proto.h b/block/opal_proto.h
index b6e352cfe982..5e8df3245eb0 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -106,6 +106,7 @@ enum opal_uid {
OPAL_ENTERPRISE_BANDMASTER0_UID,
OPAL_ENTERPRISE_ERASEMASTER_UID,
/* tables */
+ OPAL_TABLE_TABLE,
OPAL_LOCKINGRANGE_GLOBAL,
OPAL_LOCKINGRANGE_ACE_RDLOCKED,
OPAL_LOCKINGRANGE_ACE_WRLOCKED,
@@ -160,6 +161,21 @@ enum opal_token {
OPAL_STARTCOLUMN = 0x03,
OPAL_ENDCOLUMN = 0x04,
OPAL_VALUES = 0x01,
+ /* table table */
+ OPAL_TABLE_UID = 0x00,
+ OPAL_TABLE_NAME = 0x01,
+ OPAL_TABLE_COMMON = 0x02,
+ OPAL_TABLE_TEMPLATE = 0x03,
+ OPAL_TABLE_KIND = 0x04,
+ OPAL_TABLE_COLUMN = 0x05,
+ OPAL_TABLE_COLUMNS = 0x06,
+ OPAL_TABLE_ROWS = 0x07,
+ OPAL_TABLE_ROWS_FREE = 0x08,
+ OPAL_TABLE_ROW_BYTES = 0x09,
+ OPAL_TABLE_LASTID = 0x0A,
+ OPAL_TABLE_MIN = 0x0B,
+ OPAL_TABLE_MAX = 0x0C,
+
/* authority table */
OPAL_PIN = 0x03,
/* locking tokens */
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 5b7b23cb95a4..51f8034edbf7 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -136,6 +136,8 @@ static const u8 opaluid[][OPAL_UID_LENGTH] = {
/* tables */
+ [OPAL_TABLE_TABLE]
+ { 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01 },
[OPAL_LOCKINGRANGE_GLOBAL] =
{ 0x00, 0x00, 0x08, 0x02, 0x00, 0x00, 0x00, 0x01 },
[OPAL_LOCKINGRANGE_ACE_RDLOCKED] =
@@ -1107,6 +1109,29 @@ static int generic_get_column(struct opal_dev *dev,
const u8 *table,
return finalize_and_send(dev, parse_and_check_status);
}
+/*
+ * see TCG SAS 5.3.2.3 for a description of the available columns
+ *
+ * the result is provided in dev->resp->tok[4]
+ */
+static int generic_get_table_info(struct opal_dev *dev, enum opal_uid table,
+ u64 column)
+{
+ u8 uid[OPAL_UID_LENGTH];
+ const unsigned int half = OPAL_UID_LENGTH/2;
+
+ /* sed-opal UIDs can be split in two halfs:
+* first: actual table index
+* second: relative index in the table
+* so we have to get the first half of the OPAL_TABLE_TABLE and use the
+* first part of the target table as relative index into that table
+*/
+ memcpy(uid, opaluid[OPAL_TABLE_TABLE], half);
+ memcpy(uid+half, opaluid[table], half);
+
+ return generic_get_column(dev, uid, column);
+}
+
static int gen_key(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
--
2.16.1
[PATCH v2 09/11] block: sed-opal: unify retrieval of table columns
Instead of having multiple places defining the same argument list to get
a specific column of a sed-opal table, provide a generic version and
call it from those functions.
Signed-off-by: Jonas Rabenstein
diff --git a/block/opal_proto.h b/block/opal_proto.h
index e20be8258854..b6e352cfe982 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -170,6 +170,8 @@ enum opal_token {
OPAL_READLOCKED = 0x07,
OPAL_WRITELOCKED = 0x08,
OPAL_ACTIVEKEY = 0x0A,
+ /* lockingsp table */
+ OPAL_LIFECYCLE = 0x06,
/* locking info table */
OPAL_MAXRANGES = 0x04,
/* mbr control */
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 4549fa164e98..5b7b23cb95a4 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1076,6 +1076,37 @@ static int finalize_and_send(struct opal_dev *dev,
cont_fn cont)
return opal_send_recv(dev, cont);
}
+/*
+ * request @column from table @table on device @dev. On success, the column
+ * data will be available in dev->resp->tok[4]
+ */
+static int generic_get_column(struct opal_dev *dev, const u8 *table,
+ u64 column)
+{
+ int err;
+
+ err = start_opal_cmd(dev, table, opalmethod[OPAL_GET]);
+
+ add_token_u8(&err, dev, OPAL_STARTLIST);
+
+ add_token_u8(&err, dev, OPAL_STARTNAME);
+ add_token_u8(&err, dev, OPAL_STARTCOLUMN);
+ add_token_u64(&err, dev, column);
+ add_token_u8(&err, dev, OPAL_ENDNAME);
+
+ add_token_u8(&err, dev, OPAL_STARTNAME);
+ add_token_u8(&err, dev, OPAL_ENDCOLUMN);
+ add_token_u64(&err, dev, column);
+ add_token_u8(&err, dev, OPAL_ENDNAME);
+
+ add_token_u8(&err, dev, OPAL_ENDLIST);
+
+ if (err)
+ return err;
+
+ return finalize_and_send(dev, parse_and_check_status);
+}
+
static int gen_key(struct opal_dev *dev, void *data)
{
u8 uid[OPAL_UID_LENGTH];
@@ -1130,23 +1161,11 @@ static int get_active_key(struct opal_dev *dev, void
*data)
if (err)
return err;
- err = start_opal_cmd(dev, uid, opalmethod[OPAL_GET]);
- add_token_u8(&err, dev, OPAL_STARTLIST);
- add_token_u8(&err, dev, OPAL_STARTNAME);
- add_token_u8(&err, dev, 3); /* startCloumn */
- add_token_u8(&err, dev, 10); /* ActiveKey */
- add_token_u8(&err, dev, OPAL_ENDNAME);
- add_token_u8(&err, dev, OPAL_STARTNAME);
- add_token_u8(&err, dev, 4); /* endColumn */
- add_token_u8(&err, dev, 10); /* ActiveKey */
- add_token_u8(&err, dev, OPAL_ENDNAME);
- add_token_u8(&err, dev, OPAL_ENDLIST);
- if (err) {
- pr_debug("Error building get active key command\n");
+ err = generic_get_column(dev, uid, OPAL_ACTIVEKEY);
+ if (err)
return err;
- }
- return finalize_and_send(dev, get_active_key_cont);
+ return get_active_key_cont(dev);
}
static int generic_lr_enable_disable(struct opal_dev *dev,
@@ -1801,14 +1820,16 @@ static int activate_lsp(struct opal_dev *dev, void
*data)
return finalize_and_send(dev, parse_and_check_status);
}
-static int get_lsp_lifecycle_cont(struct opal_dev *dev)
+/* Determine if we're in the Manufactured Inactive or Active state */
+static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
{
u8 lc_status;
- int error = 0;
+ int err;
- error = parse_and_check_status(dev);
- if (error)
- return error;
+ err = generic_get_column(dev, opaluid[OPAL_LOCKINGSP_UID],
+OPAL_LIFECYCLE);
+ if (err)
+ return err;
lc_status = response_get_u64(&dev->parsed, 4);
/* 0x08 is Manufacured Inactive */
@@ -1821,49 +1842,19 @@ static int get_lsp_lifecycle_cont(struct opal_dev *dev)
return 0;
}
-/* Determine if we're in the Manufactured Inactive or Active state */
-static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
-{
- int err;
-
- err = start_opal_cmd(dev, opaluid[OPAL_LOCKINGSP_UID],
-opalmethod[OPAL_GET]);
-
- add_token_u8(&err, dev, OPAL_STARTLIST);
-
- add_token_u8(&err, dev, OPAL_STARTNAME);
- add_token_u8(&err, dev, 3); /* Start Column */
- add_token_u8(&err, dev, 6); /* Lifecycle Column */
- add_token_u8(&err, dev, OPAL_ENDNAME);
-
- add_token_u8(&err, dev, OPAL_STARTNAME);
- add_token_u8(&err, dev, 4); /* End Column */
- add_token_u8(&err, dev, 6); /* Lifecycle Column */
- add_token_u8(&err, dev, OPAL_ENDNAME);
-
- add_token_u8(&err, dev, OPAL_ENDLIST);
-
- if (err) {
- pr_debug("Error Building GET Lifecycle Status command\n");
- return err;
- }
-
- return finalize_and_send(dev, get_lsp_lifecycle_cont);
-}
-
-static int get_msid_cpin_pin_cont(struct opal_dev *dev)
+static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
{
const cha
[PATCH v2 05/11] block: sed-opal: print failed function address
Add function address (and if available its symbol) to the message if a
step function fails.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 30f6e46518a6..9b6f14e7aeb1 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -394,8 +394,8 @@ static int next(struct opal_dev *dev)
error = step->fn(dev, step->data);
if (error) {
- pr_debug("Error on step function: %d with error %d:
%s\n",
-state, error,
+ pr_debug("Step %d (%pS) failed wit error %d: %s\n",
+state, step->fn, error,
opal_error_to_human(error));
/* For each OPAL command we do a discovery0 then we
--
2.16.1
[PATCH v2 06/11] block: sed-opal: split generation of bytestring header and content
Split the header generation from the (normal) memcpy part if a
bytestring is copied into the command buffer. This allows in-place
generation of the bytestring content. For example, copy_from_user may be
used without an intermediate buffer.
Signed-off-by: Jonas Rabenstein
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 9b6f14e7aeb1..fc10f81d4892 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -581,14 +581,11 @@ static void add_token_u64(int *err, struct opal_dev *cmd,
u64 number)
add_token_u8(err, cmd, number >> (len * 8));
}
-static void add_token_bytestring(int *err, struct opal_dev *cmd,
-const u8 *bytestring, size_t len)
+static u8 *add_bytestring_header(int *err, struct opal_dev *cmd, size_t len)
{
size_t header_len = 1;
bool is_short_atom = true;
-
- if (*err)
- return;
+ char *start;
if (len & ~SHORT_ATOM_LEN_MASK) {
header_len = 2;
@@ -597,17 +594,27 @@ static void add_token_bytestring(int *err, struct
opal_dev *cmd,
if (!can_add(err, cmd, header_len + len)) {
pr_debug("Error adding bytestring: end of buffer.\n");
- return;
+ return NULL;
}
if (is_short_atom)
add_short_atom_header(cmd, true, false, len);
else
add_medium_atom_header(cmd, true, false, len);
-
- memcpy(&cmd->cmd[cmd->pos], bytestring, len);
+ start = &cmd->cmd[cmd->pos];
cmd->pos += len;
+ return start;
+}
+static void add_token_bytestring(int *err, struct opal_dev *cmd,
+const u8 *bytestring, size_t len)
+{
+ u8 *start;
+
+ start = add_bytestring_header(err, cmd, len);
+ if (!start)
+ return;
+ memcpy(start, bytestring, len);
}
static int build_locking_range(u8 *buffer, size_t length, u8 lr)
--
2.16.1
[PATCH] block: Change a rcu_read_{lock,unlock}_sched() pair into rcu_read_{lock,unlock}()
Since neither the RCU-protected code in blk_queue_enter() nor
blk_queue_usage_counter_release() sleeps, regular RCU protection
is sufficient. Note: scsi_device_quiesce() does not have to be
modified since it already uses synchronize_rcu().
Reported-by: Tejun Heo
Fixes: 3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work reliably")
Signed-off-by: Bart Van Assche
Cc: Hannes Reinecke
Cc: Ming Lei
Cc: Christoph Hellwig
Cc: Johannes Thumshirn
Cc: Tejun Heo
Cc: Oleksandr Natalenko
Cc: Martin Steigerwald
Cc: sta...@vger.kernel.org # v4.15
---
block/blk-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 5e88c579e896..a0f675f84f86 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -917,7 +917,7 @@ int blk_queue_enter(struct request_queue *q,
blk_mq_req_flags_t flags)
bool success = false;
int ret;
- rcu_read_lock_sched();
+ rcu_read_lock();
if (percpu_ref_tryget_live(&q->q_usage_counter)) {
/*
* The code that sets the PREEMPT_ONLY flag is
@@ -930,7 +930,7 @@ int blk_queue_enter(struct request_queue *q,
blk_mq_req_flags_t flags)
percpu_ref_put(&q->q_usage_counter);
}
}
- rcu_read_unlock_sched();
+ rcu_read_unlock();
if (success)
return 0;
--
2.16.2
Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
On Mon, Mar 19, 2018 at 04:42:09PM +0200, Artem Bityutskiy wrote: > On Mon, 2018-03-19 at 08:31 -0600, Jens Axboe wrote: > > I'm assuming that Martin will eventually queue this up. But probably > > for 4.17, then we can always flag it for a backport to stable once > > it's been thoroughly tested. > > Jens, thanks for reply. > > I wonder if folks agree that in this case we should revert > > 84676c1f21e8 genirq/affinity: assign vectors to all possible CPUs > > for v4.16. Even 84676c1f21e8 is reverted, IO failure or hang can still be triggered easily when doing CPU online/offline test. So this patchset is really needed. Thanks, Ming
RE: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
> -Original Message- > From: Artem Bityutskiy [mailto:dedeki...@gmail.com] > Sent: Monday, March 19, 2018 8:12 PM > To: h...@lst.de; Thomas Gleixner > Cc: linux-block@vger.kernel.org; snit...@redhat.com; h...@suse.de; > mr...@linux.ee; linux-s...@vger.kernel.org; don.br...@microsemi.com; > pbonz...@redhat.com; lober...@redhat.com; > kashyap.de...@broadcom.com; Jens Axboe; martin.peter...@oracle.com; > james.bottom...@hansenpartnership.com; ming@redhat.com > Subject: Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue > > On Mon, 2018-03-19 at 08:31 -0600, Jens Axboe wrote: > > I'm assuming that Martin will eventually queue this up. But probably > > for 4.17, then we can always flag it for a backport to stable once > > it's been thoroughly tested. > > Jens, thanks for reply. > > I wonder if folks agree that in this case we should revert > > 84676c1f21e8 genirq/affinity: assign vectors to all possible CPUs > > for v4.16. > > If this was a minor niche use-case regression the -stable scenario would > probably be OK. But the patch seem to miss the fact that kernel's > "possible > CPUs" notion may be way off and side effects are bad. Also it is performance issue as posted at below link, if we just use "84676c1f21e8 genirq/affinity: assign vectors to all possible CPUs". https://www.spinics.net/lists/linux-scsi/msg118301.html Performance drop was resolved using patch set (available at below link)under discussion posted by Ming. https://marc.info/?l=linux-block&m=152050646332092&w=2 Kashyap > > Christoph, Thomas, what do you think? > > Thanks, > Artem.
Re: [PATCH v5] blk-throttle: fix race between blkcg_bio_issue_check() and cgroup_rmdir()
On 3/19/18 8:45 AM, Tejun Heo wrote: > Hello, > > On Thu, Mar 15, 2018 at 09:18:55AM +0800, Joseph Qi wrote: >>> This is pure bike-shedding but offlined reads kinda weird to me, maybe >>> just offline would read better? Other than that, >>> >> Do I need to resend a new version for this? > > No idea, Jens's call. He can fix it up if he wants to while applying > but there's no harm in sending an updated version either. This got queued up last week: http://git.kernel.dk/cgit/linux-block/commit/?h=for-4.17/block&id=4c6994806f708559c2812b73501406e21ae5dcd0 for 4.17. -- Jens Axboe
Re: [PATCH v5] blk-throttle: fix race between blkcg_bio_issue_check() and cgroup_rmdir()
Hello, On Thu, Mar 15, 2018 at 09:18:55AM +0800, Joseph Qi wrote: > > This is pure bike-shedding but offlined reads kinda weird to me, maybe > > just offline would read better? Other than that, > > > Do I need to resend a new version for this? No idea, Jens's call. He can fix it up if he wants to while applying but there's no harm in sending an updated version either. Thanks. -- tejun
Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
On Mon, 2018-03-19 at 08:31 -0600, Jens Axboe wrote: > I'm assuming that Martin will eventually queue this up. But probably > for 4.17, then we can always flag it for a backport to stable once > it's been thoroughly tested. Jens, thanks for reply. I wonder if folks agree that in this case we should revert 84676c1f21e8 genirq/affinity: assign vectors to all possible CPUs for v4.16. If this was a minor niche use-case regression the -stable scenario would probably be OK. But the patch seem to miss the fact that kernel's "possible CPUs" notion may be way off and side effects are bad. Christoph, Thomas, what do you think? Thanks, Artem.
Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
On 3/19/18 5:48 AM, Bityutskiy, Artem wrote: > On Tue, 2018-03-13 at 17:42 +0800, Ming Lei wrote: >> From 84676c1f21 (genirq/affinity: assign vectors to all possible CPUs), >> one msix vector can be created without any online CPU mapped, then one >> command's completion may not be notified. > > Hello Martin, James, > > we have this patch-set and it fixes megaraid regression in v4.16. Do > you plan to mege it to v4.16-rcX? I am worried - there seem to be no > sight that this is going to me merged. They are not in the linux-next. I'm assuming that Martin will eventually queue this up. But probably for 4.17, then we can always flag it for a backport to stable once it's been thoroughly tested. -- Jens Axboe
[PATCH] rbd: fix spelling mistake: "reregisteration" -> "re-registration"
From: Colin Ian King Trivial fix to spelling mistake in rdb_warn message text Signed-off-by: Colin Ian King --- drivers/block/rbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 1e03b04819c8..7b97240ff15e 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -3887,7 +3887,7 @@ static void rbd_reregister_watch(struct work_struct *work) ret = rbd_dev_refresh(rbd_dev); if (ret) - rbd_warn(rbd_dev, "reregisteration refresh failed: %d", ret); + rbd_warn(rbd_dev, "re-registration refresh failed: %d", ret); } /* -- 2.15.1
Re: [PATCH V5 1/5] scsi: hpsa: fix selection of reply queue
On Tue, 2018-03-13 at 17:42 +0800, Ming Lei wrote: > From 84676c1f21 (genirq/affinity: assign vectors to all possible CPUs), > one msix vector can be created without any online CPU mapped, then one > command's completion may not be notified. Hello Martin, James, we have this patch-set and it fixes megaraid regression in v4.16. Do you plan to mege it to v4.16-rcX? I am worried - there seem to be no sight that this is going to me merged. They are not in the linux-next. -- Best Regards, Artem Bityutskiy - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
