Re: [PATCH] xen/scsifront: use offset_in_page() macro

[Juergen Gross]

On 22/04/17 03:21, Geliang Tang wrote:
> Use offset_in_page() macro instead of open-coding.
> 
> Signed-off-by: Geliang Tang 

Reviewed-by: Juergen Gross 


Thanks,

Juergen

Re: [PATCH 3/9] sd: Remove unecessary variable in sd_done()

[Damien Le Moal]

James,

On 4/22/17 03:42, James Bottomley wrote:
> Really, no, you're making the code less clear for no gain.  I'm fairly
> certain the compiler can optimise this without any help and when you're
> skimming the code you can easily see that the out jump is taken if you
> have a sense code that's either invalid or deferred.  After your change
> you have to glance one level deeper to come to the same conclusion.
> 
> To be clear: I wouldn't object if the original function were written
> the way you propose because we all get used to scanning code looking
> for things like this.  However, a patch to change existing code fails
> the net benefit test.

OK. Let's drop this patch then.
Thank you for the review.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 5/9] sd: Cleanup sd_done sense data handling

[Damien Le Moal]

Bart,

On 4/22/17 03:41, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> @@ -1884,8 +1886,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>>else {
>>sdkp->device->no_write_same = 1;
>>sd_config_write_same(sdkp);
>> -
>> - good_bytes = 0;
>>req->__data_len = blk_rq_bytes(req);
>>req->rq_flags |= RQF_QUIET;
>>}
> 
> This change looks fine to me but has not been described in the patch
> description? Anyway:
> 
> Reviewed-by: Bart Van Assche 

Oops. Yes, I forgot to mention that in the commit message.
(for result != 0, good_bytes is already set to 0, so that assignment is
not necessary).
Will resend.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
damien.lem...@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

Re: [PATCH 2/9] sd: Fix functions description

[Damien Le Moal]

Bart,

On 4/22/17 02:28, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> - *   Returns 0 if successful. Returns a negated errno value in case
>> - *   of error.
>> + * Returns 0 if successful. Returns a negated errno value in case
>> + * of error.
> 
> Hello Damien,
> 
> The argument name fixes are definitely welcome in my opinion. But I see
> that this patch not only fixes argument names and function descriptions
> but also includes whitespace-only changes like the above. Sorry but I
> don't like whitespace-only changes ...

Yes, I understand, you were clear about that.
The "white space" changes intent are to match the preferred format for
functions documentation (From Documentation/doc-guide/kerne-doc.rst)

  /**
   * foobar() - Brief description of foobar.
   * @arg: Description of argument of foobar.
   *
   * Longer description of foobar.
   *
   * Return: Description of return value of foobar.
   */

If that is wrong/unnecessary, then I will rewrite that patch to only
limit its scope to the argument name fixes.

Best regards.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 1/9] sd: Remove white space before EOL

[Damien Le Moal]

Bart,

On 4/22/17 02:22, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> No functional change is introduced by this patch.
> 
> Hello Damien,
> 
> The only tree for which I know that this kind of whitespace / coding style
> patches are welcome is the staging tree. In general such patches are not
> considered welcome because:
> - Such patches pollute the changelog and make it harder to use tools like
>   git annotate / git blame.
> - Such patches make it harder for distro maintainers to backport patches
>   from upstream kernels to distro (enterprise) kernels.
> - If any kernel developer has developed kernel patches for the sd driver
>   against kernel v4.11 and wants to submit these after the v4.12 merge
>   window then that developer will have to rebase (and retest) these
>   patches on top of kernel v4.12-rc1. Whitespace patches trigger really
>   ugly rebase conflicts and hence are painful for other kernel developers.
> 
> Hence my request to drop this patch.

OK. As I said, this patch was "optional".
Just wondering though how we are supposed to do code maintenance if that
kind of trivial cleanup cannot go in... (not to mention that checkpatch
keep warning about white spaces in sd.c if a patch touches something
near one of those multiple lines with white spaces).

But fine, let's drop it.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 9/9] sd_zbc: Do not write lock zones for reset

[Damien Le Moal]

Bart,

On 4/22/17 02:15, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> @@ -250,11 +249,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
>>/* Unaligned request */
>>return BLKPREP_KILL;
>>  
>> - /* Do not allow concurrent reset and writes */
>> - if (sdkp->zones_wlock &&
>> - test_and_set_bit(zno, sdkp->zones_wlock))
>> - return BLKPREP_DEFER;
>> -
> 
> Hello Damien,
> 
> Since concurrent zone resets and writes are easy to detect and since these
> indicate a bug in the application that submits these, should an error
> message
> be logged if that happens? Otherwise this patch looks fine to me.

Yes, we could easily add a warning, but if we consider the 3 possible
cases of reset+write combinations:
1) Reset comes in while a write is already in the queue: The write will
succeed (assuming it is directed at the zone write pointer) and the
reset will also succeed.
2) A write directed at the current write pointer comes in while a reset
is in the queue: the write will fail.
3) A write directed at the beginning of the zone comes in while a reset
is in the queue: the write will succeed.

The warning would trigger only in case (1), but that case may be
actually intentional by the user. Case (2) will get an "indirect"
warning through the write align error, which clearly signals a bug to
the user. For case (3), that may also be intentional by the user (not
recommended though).

So I am afraid that a warning may trigger false-positives and may not be
that useful in the end.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
damien.lem...@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

Re: [PATCH 7/9] sd_zbc: Rename sd_zbc_setup_write_cmnd

[Damien Le Moal]

Bart,

On 4/22/17 02:07, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> Rename sd_zbc_setup_write_cmnd() to sd_zbc_write_lock_zone() to be
>> clear about what the function actually does. To be consistent, also
>> rename sd_zbc_cancel_write_cmnd() to sd_zbc_write_unlock_zone().
>> 
>> [ ... ]
>>/*
>> -  * resid is optional but mostly filled in.  When it's unused,
>> +  * resid is optional but mostly filled in. When it's unused,
>> * its value is zero, so we assume the whole buffer transferred
>> */
> 
> Is this whitespace-only change useful? Otherwise this patch looks fine
> to me. Hence:
> 
> Reviewed-by: Bart Van Assche 

Ooops... No it is not. I will resend a v2.
Thanks.


-- 
Damien Le Moal,
Western Digital

Re: [PATCH 6/9] scsi: Improve scsi_get_sense_info_fld

[Damien Le Moal]

Bart,

On 4/22/17 01:58, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lem...@wdc.com wrote:
>> @@ -2363,42 +2365,32 @@ EXPORT_SYMBOL(scsi_command_normalize_sense);
>>   * Return value:
>>   *   1 if information field found, 0 if not found.
>>   */
>> -int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>> - u64 * info_out)
>> +bool scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>> +  u64 * info_out)
> 
> Hello Damien,
> 
> If you have to repost this patch please address the checkpatch complaints
> of the following category triggered by the above declaration:
> 
> ERROR: "foo * bar" should be "foo *bar"
> 
> Otherwise this patch looks fine to me. Hence:
> 
> Reviewed-by: Bart Van Assche 

OK. Will do.

-- 
Damien Le Moal,
Western Digital

[PATCH] scsi: mvumi: remove code handling zero scsi_sg_count(scmd) case

[Alexey Khoroshilov]

As Christoph Hellwig noted, SCSI commands that transfer data
always have a SG entry. The patch removes dead code in
mvumi_make_sgl(),  mvumi_complete_cmd() and mvumi_timed_out()
that handle zero scsi_sg_count(scmd) case.

Also the patch adds pci_unmap_sg() on failure path in mvumi_make_sgl().

Signed-off-by: Alexey Khoroshilov 
---
 drivers/scsi/mvumi.c | 85 
 1 file changed, 26 insertions(+), 59 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 247df5e79b71..fe97401ad192 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -210,39 +210,27 @@ static int mvumi_make_sgl(struct mvumi_hba *mhba, struct 
scsi_cmnd *scmd,
unsigned int sgnum = scsi_sg_count(scmd);
dma_addr_t busaddr;
 
-   if (sgnum) {
-   sg = scsi_sglist(scmd);
-   *sg_count = pci_map_sg(mhba->pdev, sg, sgnum,
-   (int) scmd->sc_data_direction);
-   if (*sg_count > mhba->max_sge) {
-   dev_err(>pdev->dev, "sg count[0x%x] is bigger "
-   "than max sg[0x%x].\n",
-   *sg_count, mhba->max_sge);
-   return -1;
-   }
-   for (i = 0; i < *sg_count; i++) {
-   busaddr = sg_dma_address([i]);
-   m_sg->baseaddr_l = cpu_to_le32(lower_32_bits(busaddr));
-   m_sg->baseaddr_h = cpu_to_le32(upper_32_bits(busaddr));
-   m_sg->flags = 0;
-   sgd_setsz(mhba, m_sg, cpu_to_le32(sg_dma_len([i])));
-   if ((i + 1) == *sg_count)
-   m_sg->flags |= 1U << mhba->eot_flag;
-
-   sgd_inc(mhba, m_sg);
-   }
-   } else {
-   scmd->SCp.dma_handle = scsi_bufflen(scmd) ?
-   pci_map_single(mhba->pdev, scsi_sglist(scmd),
-   scsi_bufflen(scmd),
-   (int) scmd->sc_data_direction)
-   : 0;
-   busaddr = scmd->SCp.dma_handle;
+   sg = scsi_sglist(scmd);
+   *sg_count = pci_map_sg(mhba->pdev, sg, sgnum,
+  (int) scmd->sc_data_direction);
+   if (*sg_count > mhba->max_sge) {
+   dev_err(>pdev->dev,
+   "sg count[0x%x] is bigger than max sg[0x%x].\n",
+   *sg_count, mhba->max_sge);
+   pci_unmap_sg(mhba->pdev, sg, sgnum,
+(int) scmd->sc_data_direction);
+   return -1;
+   }
+   for (i = 0; i < *sg_count; i++) {
+   busaddr = sg_dma_address([i]);
m_sg->baseaddr_l = cpu_to_le32(lower_32_bits(busaddr));
m_sg->baseaddr_h = cpu_to_le32(upper_32_bits(busaddr));
-   m_sg->flags = 1U << mhba->eot_flag;
-   sgd_setsz(mhba, m_sg, cpu_to_le32(scsi_bufflen(scmd)));
-   *sg_count = 1;
+   m_sg->flags = 0;
+   sgd_setsz(mhba, m_sg, cpu_to_le32(sg_dma_len([i])));
+   if ((i + 1) == *sg_count)
+   m_sg->flags |= 1U << mhba->eot_flag;
+
+   sgd_inc(mhba, m_sg);
}
 
return 0;
@@ -1350,21 +1338,10 @@ static void mvumi_complete_cmd(struct mvumi_hba *mhba, 
struct mvumi_cmd *cmd,
break;
}
 
-   if (scsi_bufflen(scmd)) {
-   if (scsi_sg_count(scmd)) {
-   pci_unmap_sg(mhba->pdev,
-   scsi_sglist(scmd),
-   scsi_sg_count(scmd),
-   (int) scmd->sc_data_direction);
-   } else {
-   pci_unmap_single(mhba->pdev,
-   scmd->SCp.dma_handle,
-   scsi_bufflen(scmd),
-   (int) scmd->sc_data_direction);
-
-   scmd->SCp.dma_handle = 0;
-   }
-   }
+   if (scsi_bufflen(scmd))
+   pci_unmap_sg(mhba->pdev, scsi_sglist(scmd),
+scsi_sg_count(scmd),
+(int) scmd->sc_data_direction);
cmd->scmd->scsi_done(scmd);
mvumi_return_cmd(mhba, cmd);
 }
@@ -2171,19 +2148,9 @@ static enum blk_eh_timer_return mvumi_timed_out(struct 
scsi_cmnd *scmd)
scmd->result = (DRIVER_INVALID << 24) | (DID_ABORT << 16);
scmd->SCp.ptr = NULL;
if (scsi_bufflen(scmd)) {
-   if (scsi_sg_count(scmd)) {
-   pci_unmap_sg(mhba->pdev,
-   scsi_sglist(scmd),
-   scsi_sg_count(scmd),
-   (int)scmd->sc_data_direction);
-   } else {
-   pci_unmap_single(mhba->pdev,
-  

Re: [PATCH 3/4] nfs: remove the objlayout driver

[Boaz Harrosh]

On 04/21/2017 05:00 PM, Trond Myklebust wrote:
> Maintenance is not development. It’s about doing all the followup
> _after_ the feature is declared to be developed. That’s been missing
> for quite some time in the case of the OSD pNFS code, which is why
> I’m not even bothering to consider staging. If you are saying you are
> still maintaining exofs, and want to continue doing so, then great,
> but note that there is a file called BUGS in that directory that has
> been untouched since 2008, and that’s why I thing staging is a good
> idea.
> 

No, the BUGS file is just stale. As you said was not ever updated. All
the bugs (1) in there do no longer exist for a long long time.
I will send a patch to remove the file.

Yes I maintain this fs. It has the complete fixture list, of what was
first intended. I keep running it every major kernel release to make
sure it actually runs, and there are no regressions.

If this FS stands in the way of any new development please anyone
contact me and I will help in the conversion. Of exofs and the
osd-uld.

Thanks
Boaz

Re: [PATCH] scsi: mvumi: add check for dma mapping errors

[Christoph Hellwig]

On Sat, Apr 22, 2017 at 02:17:50AM +0300, Alexey Khoroshilov wrote:
>   } else {
> - scmd->SCp.dma_handle = scsi_bufflen(scmd) ?
> - pci_map_single(mhba->pdev, scsi_sglist(scmd),
> - scsi_bufflen(scmd),
> - (int) scmd->sc_data_direction)
> - : 0;
> + if (!scsi_bufflen(scmd))
> + return -1;
> + scmd->SCp.dma_handle = pci_map_single(mhba->pdev,
> + scsi_sglist(scmd),
> + scsi_bufflen(scmd),
> + (int) scmd->sc_data_direction);
> + if (pci_dma_mapping_error(mhba->pdev, scmd->SCp.dma_handle))
> + return -1;

This looks completely broken.  Why would you DMA map the in-memory
struct scatterlist?  It has no meaning for the hardware.

In fact this whole branch (and the equivalent in the unmap path)
are dead - SCSI commands that transfer data always have a SG entry.

So the right fix is to remove the !scsi_sg_count(scmd) map/unmap
path.

Re: [PATCH 3/4] scsi: pmcraid: fix endianess sparse annotations

[Christoph Hellwig]

>   mb();
> - iowrite32(le32_to_cpu(cmd->ioa_cb->ioarcb.ioarcb_bus_addr),
> -   pinstance->ioarrin);
> + iowrite32(le64_to_cpu(cmd->ioa_cb->ioarcb.ioarcb_bus_addr), 
> pinstance->ioarrin);

It really seems like some of these fields should be swapped once
and store in an in-memory structure.

But for now this patch looks fine to me:

Reviewed-by: Christoph Hellwig 

Re: [PATCH 4/4] scsi: pmcraid: fix minor sparse warnings

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 

Re: [PATCH 2/4] scsi: pmcraid: fix lock imbalance in pmcraid_reset_reload()

[Christoph Hellwig]

On Thu, Apr 20, 2017 at 07:54:46PM +0200, Arnd Bergmann wrote:
> sparse found a bug that has always been present since the driver
> was merged:
> 
> drivers/scsi/pmcraid.c:2353:12: warning: context imbalance in 
> 'pmcraid_reset_reload' - different lock contexts for basic block
> 
> This adds the missing unlock at the end of the function. I could
> not figure out if this will happen in practice, but I could not
> prove that it doesn't happen, so to be on the safe side, let's
> backport this fix.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 89a368104150 ("[SCSI] pmcraid: PMC-Sierra MaxRAID driver to support 
> 6Gb/s SAS RAID controller")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/scsi/pmcraid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index 096c704ca39a..35087e94c2ad 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -2411,8 +2411,11 @@ static int pmcraid_reset_reload(
>   scsi_unblock_requests(pinstance->host);
>   if (pinstance->ioa_state == target_state)
>   reset = 0;
> + spin_lock_irqsave(pinstance->host->host_lock, lock_flags);
>   }
>  
> + spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
> +

This looks weird.  Using a proper goto label to unwind seem like the
best way to go, e.g. this patch:

---
>From 8c58854f345bd87789b68eba2b2f72d0cac952fa Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Sun, 23 Apr 2017 10:33:23 +0200
Subject: pmcraid: fix lock imbalance in pmcraid_reset_reload()

sparse found a bug that has always been present since the driver was
merged:

drivers/scsi/pmcraid.c:2353:12: warning: context imbalance in 
'pmcraid_reset_reload' - different lock contexts for basic block

Fix this by using a common unlock goto label, and also reduce the
indentation level in the function.

Signed-off-by: Christoph Hellwig 
Reported-by: Arnd Bergmann 
---
 drivers/scsi/pmcraid.c | 59 --
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 49e70a383afa..3cc858f45838 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -2373,46 +2373,43 @@ static int pmcraid_reset_reload(
spin_lock_irqsave(pinstance->host->host_lock, lock_flags);
 
if (pinstance->ioa_state == IOA_STATE_DEAD) {
-   spin_unlock_irqrestore(pinstance->host->host_lock,
-  lock_flags);
pmcraid_info("reset_reload: IOA is dead\n");
-   return reset;
-   } else if (pinstance->ioa_state == target_state) {
+   goto out_unlock;
+   }
+
+   if (pinstance->ioa_state == target_state) {
reset = 0;
+   goto out_unlock;
}
}
 
-   if (reset) {
-   pmcraid_info("reset_reload: proceeding with reset\n");
-   scsi_block_requests(pinstance->host);
-   reset_cmd = pmcraid_get_free_cmd(pinstance);
-
-   if (reset_cmd == NULL) {
-   pmcraid_err("no free cmnd for reset_reload\n");
-   spin_unlock_irqrestore(pinstance->host->host_lock,
-  lock_flags);
-   return reset;
-   }
+   pmcraid_info("reset_reload: proceeding with reset\n");
+   scsi_block_requests(pinstance->host);
+   reset_cmd = pmcraid_get_free_cmd(pinstance);
+   if (reset_cmd == NULL) {
+   pmcraid_err("no free cmnd for reset_reload\n");
+   goto out_unlock;
+   }
 
-   if (shutdown_type == SHUTDOWN_NORMAL)
-   pinstance->ioa_bringdown = 1;
+   if (shutdown_type == SHUTDOWN_NORMAL)
+   pinstance->ioa_bringdown = 1;
 
-   pinstance->ioa_shutdown_type = shutdown_type;
-   pinstance->reset_cmd = reset_cmd;
-   pinstance->force_ioa_reset = reset;
-   pmcraid_info("reset_reload: initiating reset\n");
-   pmcraid_ioa_reset(reset_cmd);
-   spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
-   pmcraid_info("reset_reload: waiting for reset to complete\n");
-   wait_event(pinstance->reset_wait_q,
-  !pinstance->ioa_reset_in_progress);
+   pinstance->ioa_shutdown_type = shutdown_type;
+   pinstance->reset_cmd = reset_cmd;
+   pinstance->force_ioa_reset = reset;
+   pmcraid_info("reset_reload: initiating reset\n");
+   pmcraid_ioa_reset(reset_cmd);
+   spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
+   pmcraid_info("reset_reload: waiting for reset to complete\n");

Re: [PATCH] scsi: pmcraid: use normal copy_from_user

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig