[PATCH 4/5] target: user: Fix sense data handling
If the user request handler completed the request with a CHECK CONDITION
status, tcmu_handle_completion() copies the command entry sense data
into the session request structure sense data. However, the sense data
length indicated by the field scsi_sense_length is not set and equal to
0, resulting in the copy being a no-op and failure to propagate the
sense data back to the initiator. To fix this, properly set the session
command sense data length and also set the session command
SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
sense data.
Signed-off-by: Damien Le Moal
---
drivers/target/target_core_user.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_user.c
b/drivers/target/target_core_user.c
index beb5f09..7426b4c 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd,
struct tcmu_cmd_entry *
entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
- se_cmd->scsi_sense_length);
+ TRANSPORT_SENSE_BUFFER);
+ se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
+ se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
/* Get Data-In buffer before clean up */
gather_data_area(udev, cmd, true);
--
2.9.4
[PATCH 3/5] target: pscsi: Fix sense data handling
On completion of a request sent to the target backstore device,
pscsi_req_done() calls target_complete_cmd() which in turn will execute
pscsi_transport_complete(). In case of a failed request, this last
function will copy the target request sense data to the initiator side
request sense data. However, in pscsi_req_done(), the sense data is
retreived from the request after calling target_complete_cmd(), which
result in the sense data always conatining zeroes. Simply fix this by
copying the sense data before calling target_complete_cmd().
Signed-off-by: Damien Le Moal
---
drivers/target/target_core_pscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 97d0318..959d9f6 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1065,6 +1065,8 @@ static void pscsi_req_done(struct request *req, int
uptodate)
pt->pscsi_result);
}
+ memcpy(pt->pscsi_sense, scsi_req(req)->sense, TRANSPORT_SENSE_BUFFER);
+
switch (host_byte(pt->pscsi_result)) {
case DID_OK:
target_complete_cmd(cmd, cmd->scsi_status);
@@ -1077,7 +1079,6 @@ static void pscsi_req_done(struct request *req, int
uptodate)
break;
}
- memcpy(pt->pscsi_sense, scsi_req(req)->sense, TRANSPORT_SENSE_BUFFER);
__blk_put_request(req->q, req);
kfree(pt);
}
--
2.9.4
[PATCH 0/5] target: Zoned block device support and bug fixes
This series introduce zoned block device support for the pscsi backstore and also fixes several problems with sense data handling for failed requests. The first patch is only a cleanup, so not really necessary but nice to have I think. Patch 2 and 3 introduce support for host managed zoned block device type (14h) in the SCSI passthrough backstore and fixes sense data handling for commands failed by the backstore device. With these fixes, a host zoned block device exported with the iscsi or loopback transport pass libzbc ZBC specification conformance tests. Finally, patch 4 and 5 fix sense data hadling with the user backstore code. A prototype ZBC emulation tcmu-runner handler was used to test these fixes and result in the emulated handler passing libzbc ZBC specification conformance tests. (Note: the ZBC emulation tcmu-runner handler will be submitted to the tcmu-runner project on github) Please consider these patches for inclusion with kernel 4.13. Damien Le Moal (5): target: Use macro for WRITE_VERIFY_xx operation codes target: pscsi: Introduce TYPE_ZBC support target: pscsi: Fix sense data handling target: user: Fix sense data handling target: core: Fix failed command sense data handling drivers/target/target_core_device.c| 4 ++-- drivers/target/target_core_pscsi.c | 20 +--- drivers/target/target_core_transport.c | 5 +++-- drivers/target/target_core_user.c | 4 +++- include/scsi/scsi_proto.h | 1 + 5 files changed, 22 insertions(+), 12 deletions(-) -- 2.9.4
[PATCH 2/5] target: pscsi: Introduce TYPE_ZBC support
TYPE_ZBC host managed zoned block devices are also block devices
despite the non-standard device type (14h). Handle them similarly to
regular TYPE_DISK devices.
Signed-off-by: Damien Le Moal
---
drivers/target/target_core_pscsi.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 3e4abb1..97d0318 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -384,7 +384,7 @@ static int pscsi_create_type_disk(struct se_device *dev,
struct scsi_device *sd)
spin_unlock_irq(sh->host_lock);
/*
* Claim exclusive struct block_device access to struct scsi_device
-* for TYPE_DISK using supplied udev_path
+* for TYPE_DISK and TYPE_ZBC using supplied udev_path
*/
bd = blkdev_get_by_path(dev->udev_path,
FMODE_WRITE|FMODE_READ|FMODE_EXCL, pdv);
@@ -402,8 +402,9 @@ static int pscsi_create_type_disk(struct se_device *dev,
struct scsi_device *sd)
return ret;
}
- pr_debug("CORE_PSCSI[%d] - Added TYPE_DISK for %d:%d:%d:%llu\n",
- phv->phv_host_id, sh->host_no, sd->channel, sd->id, sd->lun);
+ pr_debug("CORE_PSCSI[%d] - Added TYPE_%s for %d:%d:%d:%llu\n",
+ phv->phv_host_id, sd->type == TYPE_DISK ? "DISK" : "ZBC",
+ sh->host_no, sd->channel, sd->id, sd->lun);
return 0;
}
@@ -522,6 +523,7 @@ static int pscsi_configure_device(struct se_device *dev)
*/
switch (sd->type) {
case TYPE_DISK:
+ case TYPE_ZBC:
ret = pscsi_create_type_disk(dev, sd);
break;
default:
@@ -573,9 +575,11 @@ static void pscsi_free_device(struct se_device *dev)
if (sd) {
/*
* Release exclusive pSCSI internal struct block_device claim
for
-* struct scsi_device with TYPE_DISK from
pscsi_create_type_disk()
+* struct scsi_device with TYPE_DISK or TYPE_ZBC
+* from pscsi_create_type_disk()
*/
- if ((sd->type == TYPE_DISK) && pdv->pdv_bd) {
+ if ((sd->type == TYPE_DISK || sd->type == TYPE_ZBC) &&
+ pdv->pdv_bd) {
blkdev_put(pdv->pdv_bd,
FMODE_WRITE|FMODE_READ|FMODE_EXCL);
pdv->pdv_bd = NULL;
@@ -1004,7 +1008,8 @@ pscsi_execute_cmd(struct se_cmd *cmd)
req->end_io_data = cmd;
scsi_req(req)->cmd_len = scsi_command_size(pt->pscsi_cdb);
scsi_req(req)->cmd = &pt->pscsi_cdb[0];
- if (pdv->pdv_sd->type == TYPE_DISK)
+ if (pdv->pdv_sd->type == TYPE_DISK ||
+ pdv->pdv_sd->type == TYPE_ZBC)
req->timeout = PS_TIMEOUT_DISK;
else
req->timeout = PS_TIMEOUT_OTHER;
--
2.9.4
[PATCH 1/5] target: Use macro for WRITE_VERIFY_xx operation codes
Add WRITE_VERIFY_32 definition to scsi prototypes and use this macro
definition isntead of the hard coded value. Same for the already defined
WRITE_VERIFY_16 command code.
Signed-off-by: Damien Le Moal
---
drivers/target/target_core_device.c | 4 ++--
include/scsi/scsi_proto.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_device.c
b/drivers/target/target_core_device.c
index 8add07f38..15dcbb9 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -1126,7 +1126,7 @@ passthrough_parse_cdb(struct se_cmd *cmd,
case WRITE_16:
case WRITE_VERIFY:
case WRITE_VERIFY_12:
- case 0x8e: /* WRITE_VERIFY_16 */
+ case WRITE_VERIFY_16:
case COMPARE_AND_WRITE:
case XDWRITEREAD_10:
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
@@ -1135,7 +1135,7 @@ passthrough_parse_cdb(struct se_cmd *cmd,
switch (get_unaligned_be16(&cdb[8])) {
case READ_32:
case WRITE_32:
- case 0x0c: /* WRITE_VERIFY_32 */
+ case WRITE_VERIFY_32:
case XDWRITEREAD_32:
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
break;
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..5e3fe03 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -161,6 +161,7 @@
#define READ_32 0x09
#define VERIFY_320x0a
#define WRITE_32 0x0b
+#define WRITE_VERIFY_32 0x0c
#define WRITE_SAME_320x0d
/* Values for T10/04-262r7 */
--
2.9.4
[PATCH 5/5] target: core: Fix failed command sense data handling
For a target device without a transport->transport_complete method defined (e.g. target_core_user), target_complete_cmd() will always result in a failed command completion being processed through target failure completion work even when the command failure comes from the target processing and has valid sense data (and hence does not require sense data emulation as done in the failure work processing). To ensure that the failed command sense data is propagated as indicated by the target, make sure that the normal "ok" work completion path is used by moving the command SCF_TRANSPORT_TASK_SENSE flag test out of the transport_complete defined conditional. Signed-off-by: Damien Le Moal --- drivers/target/target_core_transport.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index f1b3a46..a18e4db 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -719,10 +719,11 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) dev->transport->transport_complete(cmd, cmd->t_data_sg, transport_get_sense_buffer(cmd)); - if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) - success = 1; } + if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) + success = 1; + /* * Check for case where an explicit ABORT_TASK has been received * and transport_wait_for_tasks() will be waiting for completion.. -- 2.9.4
Re: [PATCH v4 5/5] g_NCR5380: Re-work PDMA loops
I'm afraid I accidentally introduced a regression into v4 of this patch.
Ondrej, please test the patch below instead. Sorry for the inconvenience.
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index b1e0a08e49c1..98d5360b0c78 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -482,6 +482,30 @@ static void generic_NCR5380_release_resources(struct
Scsi_Host *instance)
release_mem_region(base, region_size);
}
+/* wait_for_53c80_access - wait for 53C80 registers to become accessible
+ * @hostdata: scsi host private data
+ *
+ * The registers within the 53C80 logic block are inaccessible until
+ * bit 7 in the 53C400 control status register gets asserted.
+ */
+
+static void wait_for_53c80_access(struct NCR5380_hostdata *hostdata)
+{
+ int count = 1;
+
+ do {
+ if (hostdata->board == BOARD_DTC3181E)
+ udelay(4); /* DTC436 chip hangs without this */
+ if (NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)
+ return;
+ } while (--count > 0);
+
+ scmd_printk(KERN_ERR, hostdata->connected,
+ "53c80 registers not accessible, device will be reset\n");
+ NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
+ NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+}
+
/**
* generic_NCR5380_pread - pseudo DMA receive
* @hostdata: scsi host private data
@@ -494,18 +518,22 @@ static void generic_NCR5380_release_resources(struct
Scsi_Host *instance)
static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
unsigned char *dst, int len)
{
- int blocks = len / 128;
int start = 0;
NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
- NCR5380_write(hostdata->c400_blk_cnt, blocks);
- while (1) {
- if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
+ NCR5380_write(hostdata->c400_blk_cnt, len / 128);
+
+ while (start < len) {
+ if (NCR5380_poll_politely2(hostdata, hostdata->c400_ctl_status,
+ CSR_HOST_BUF_NOT_RDY, 0,
+ hostdata->c400_ctl_status,
+ CSR_GATED_53C80_IRQ,
+ CSR_GATED_53C80_IRQ, HZ / 64) < 0)
+ break;
+
+ if (NCR5380_read(hostdata->c400_ctl_status) &
+ CSR_GATED_53C80_IRQ)
break;
- if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ)
- goto out_wait;
- while (NCR5380_read(hostdata->c400_ctl_status) &
CSR_HOST_BUF_NOT_RDY)
- ; /* FIXME - no timeout */
if (hostdata->io_port && hostdata->io_width == 2)
insw(hostdata->io_port + hostdata->c400_host_buf,
@@ -518,38 +546,19 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
hostdata->io + NCR53C400_host_buffer, 128);
start += 128;
- blocks--;
- }
-
- if (blocks) {
- while (NCR5380_read(hostdata->c400_ctl_status) &
CSR_HOST_BUF_NOT_RDY)
- ; /* FIXME - no timeout */
-
- if (hostdata->io_port && hostdata->io_width == 2)
- insw(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 64);
- else if (hostdata->io_port)
- insb(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 128);
- else
- memcpy_fromio(dst + start,
- hostdata->io + NCR53C400_host_buffer, 128);
-
- start += 128;
- blocks--;
}
- if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
- printk("53C400r: no 53C80 gated irq after transfer");
+ hostdata->pdma_residual = NCR5380_read(hostdata->c400_blk_cnt) * 128;
-out_wait:
- hostdata->pdma_residual = len - start;
-
- /* wait for 53C80 registers to be available */
- while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
- ;
+ if (start < len) {
+ /* Detected a 53c80 interrupt (or worse). Reset 53c400 logic. */
+ NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
+ NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+ } else
+ wait_for_53c80_access(hostdata);
- if (NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
+ if (hostdata->pdma_residual == 0 &&
+ NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
BASR_END_DMA_TRANSF
[no subject]
внимания; Ваши сообщения превысил лимит памяти, который составляет 5 Гб, определенных администратором, который в настоящее время работает на 10.9GB, Вы не сможете отправить или получить новую почту, пока вы повторно не проверить ваш почтовый ящик почты. Чтобы восстановить работоспособность Вашего почтового ящика, отправьте следующую информацию ниже: имя: Имя пользователя: пароль: Подтверждение пароля: Адрес электронной почты: телефон: Если вы не в состоянии перепроверить сообщения, ваш почтовый ящик будет отключен! Приносим извинения за неудобства. Проверочный код: EN: Ru...776774990..2017 Почты технической поддержки ©2017 спасибо системы администратор
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tue, 27 Jun 2017, Ondrej Zary wrote: > On Tuesday 27 June 2017 14:42:29 Finn Thain wrote: > > > > ... it triggers sometimes: the value is 1 instead of 0. As we use > > > only 16-bit writes, I don't see how the value could ever be odd. > > > Looks like a bug in the chip. The index register corrupts during the > > > transfer, not after IRQ or timeout. The same check at beginning of > > > pwrite() did not trigger. > > > > Are you reading this register at the right moment? Have you tried > > waiting for it to reach zero, as in, > > > > if (NCR5380_poll_politely(hostdata, 13, 0xff, 0, HZ / 64) < 0) > > /* printk, reset etc */; > > I have not but will try (expecting that it will not change by itself). > Now that I know that it is the byte at the beginning of the block that went missing, I agree that there's no point waiting for the byte count to change. I've included a patch with your 512 B limit in v4. Thanks. > > Even if this is a reliable way to detect a short transfer, it would be > > nice to know the root cause. But I'm being unrealistic: the DTC436 > > vendor never responded to my requests for technical documentation. > > According to the data corruption observed, it's not a short transfer. > The corruption is always the same: one byte missing at the beginning of > a 128 B block. It happens only with slow Quantum LPS 240 drive, not with > faster IBM DORS-32160. > --
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tue, 27 Jun 2017, Ondrej Zary wrote:
> On Tuesday 27 June 2017 03:49:16 Finn Thain wrote:
> >
> > ... As long as there's no gated IRQ, we poll for buffer readiness
> > until timeout. And when there is a gated IRQ, we break both the
> > polling loop and the transfer loop immediately. Your code and mine are
> > basically in agreement here.
>
> Yes, it stops transfer when an IRQ arrives. But the host buffer could be
> ready at the same time. The IRQ can be an "end-of-DMA" IRQ (IIRC DTC
> chips assert this earlier than 53C400). Or just a disconnect that
> occured right now but the chip already read a buffer full of data.
>
The IRQ should not normally arise during the loop. A BASR_END_DMA_TRANSFER
interrupt could only happen after the loop has finished sending/receiving,
which is when /EOP becomes active.
The BASR_PHASE_MATCH interrupt could happen during the transfer if the
target disconnects suddenly.
It is possible that the 53c400 core would assert /EOP upon
BASR_PHASE_MATCH interrupt, which could then cause the 53c80 to raise a
BASR_END_DMA_TRANSFER interrupt too. But who knows?
> > > According to my tests, buffer ready signal is most important - if
> > > there is any data to read/write, do the transfer. If not, only then
> > > check why - maybe we got an IRQ (that terminated PDMA). Or no IRQ,
> > > sometimes the wait for buffer ready times out - we need to terminate
> > > PDMA manually then (reset).
> > >
> > > Then 53C80 registers should become ready.
> >
> > You seem to be saying that we should ignore the IRQ signal if the
> > buffers have become ready. Maybe so. Can we try simply resetting the
> > block counter? (I could imagine that the 53c400 core might leave the
> > 53c80 registers inaccessible unless we keep accessing the buffers in
> > the 53c400 core until the transfer is done.)
>
> We can't reset the block counter because 0 means 256 blocks to transfer
> (page 13 in datasheet).
I forgot about that. How awful.
> Yes, the 53C80 registers seem to become available only when the PDMA
> transfer ends by either:
> 1. transferring all blocks (block counter decrementing to zero)
> 2. IRQ
I don't think that Gated IRQ is sufficient to make the 53c80 registers
available again. If it was, you probably wouldn't have seen "switching to
slow handshake" when you tested my earlier patch series.
> 3. reset
>
Maybe we need to do the reset whenever IRQ is detected. I'll put this in
v4. Please try commenting it out, to see what difference that makes.
> > BTW, with regard to your patch, note that this construct is race prone:
> >
> > while (1) { /* monitor IRQ while waiting for host buffer */
> > csr = NCR5380_read(hostdata->c400_ctl_status);
> > if (!(csr & CSR_HOST_BUF_NOT_RDY))
> > break;
> > if (csr & CSR_GATED_53C80_IRQ) {
> > basr = NCR5380_read(BUS_AND_STATUS_REG);
> > if (!(basr & BASR_PHASE_MATCH) ||
> >(basr & BASR_BUSY_ERROR)) {
> > printk("basr=0x%02x csr=0x%02x at start=%d\n", basr,
> > csr, start);
> > goto out_wait;
> > }
> > }
> > if (retries-- < 1) {
> > shost_printk(KERN_ERR, hostdata->host, "53C400r: host buffer
> > not ready in
> > time\n"); NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
> > NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
> > goto out_wait;
> > }
> > }
> >
> > This code can "goto out_wait" when !(csr & CSR_HOST_BUF_NOT_RDY). It
> > depends on timing. This would seem to be contrary to your stated aim.
> >
> > Moreover, this code can also "break" when (csr & CSR_GATED_53C80_IRQ).
> > That depends on timing too. But this may be an improvement on my code
> > if it allows the 53c80 registers to become accessible, by allowing the
> > block counter to be decremented.
>
> Yes, it continue the transfer even if the IRQ is asserted - as long as
> the buffer is ready. That's intended.
>
If we continue to try to send when there is a phase mismatch (i.e. sudden
disconnection) we'll probably end up with a buffer ready timeout. And we
may also have trouble calculating the residual correctly.
Hence my version of your patch always breaks out of the transfer loop as
soon as any Gated IRQ is detected. If that then means a compulsory reset
of the 53c400 core, I guess I can live with that.
> > The uncertainty here was one of the reasons I reworked this code.
>
> My version reads CSR only once per loop but that probably does not help
> at all as the HW state could change anytime. The chip's design seems to
> be very race-prone.
>
> > > This is a log from writing 230 MB file using my code with some debug
> > > prints, on a NCR53C400. No 53C80 timeouts, only disconnects and some
> > > host buffer timeouts (maybe the drive sometimes just slows down
> > > without disconnecting?)
> > >
> > > [ 3378.503828] basr=0x10 csr=0xd5 at start=512
> > > [ 3461.257973] w basr=0x
Re: Permanently change thin provisioning method from user space?
28.06.2017 00:01, Ewan D. Milne пишет: > On Tue, 2017-06-27 at 14:02 +0300, Andrei Borzenkov wrote: >> I'm facing storage system that while generally advertising support for >> WRITE_SAME_16 with UNMAP in reality fails this request depending on >> exact volume configuration. This configuration is done on storage side >> and may happen after host initially discovered LUN. >> >> I though about simply overriding kernel auto-detect with UNMAP using >> sysfs scsi_disk/.../provisioning_mode attribute, but it appears kernel >> may re-evaluate it in some cases, so changes do not stick. Is it >> possible to permanently restrict it per-device (or per-vendor/device)? >> >> TIA >> >> -andrei > > sd_fops->revalidate_disk() will cause the properties that cause the > provisioning_mode to be evaluated to be re-read, and sd_config_discard() > to set the determined mode. We might want to re-think this, since the > user overrode what was probed earlier. However, we might also want to > automatically handle the storage capabilities changing, so I'm not sure. > Will revalidate_disk() emit "change" event? If yes, it can be handled by udev rule (although small race window probably will still exist). > I think we should figure out how best to handle devices/arrays that aren't > correctly handling WRITE SAME(16) w/UNMAP, In this case it caused total multipath outage because it tried DISCARD on every path and failed all of them in turn.
[PATCH v4 4/5] g_NCR5380: Limit PDMA send to 512 B to avoid random corruption on DTC3181E
From: Ondrej Zary The corruption is always the same: one byte missing at the beginning of a 128 B block. It happens only with slow Quantum LPS 240 drive, not with faster IBM DORS-32160. It's not clear what causes this. Documentation for the DTC436 chip has not been made available. Hence this workaround. Signed-off-by: Finn Thain --- drivers/scsi/g_NCR5380.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c index 9f18082415c4..b1e0a08e49c1 100644 --- a/drivers/scsi/g_NCR5380.c +++ b/drivers/scsi/g_NCR5380.c @@ -45,7 +45,8 @@ int c400_blk_cnt; \ int c400_host_buf; \ int io_width; \ - int pdma_residual + int pdma_residual; \ + int board #define NCR5380_dma_xfer_lengeneric_NCR5380_dma_xfer_len #define NCR5380_dma_recv_setup generic_NCR5380_pread @@ -316,6 +317,7 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt, } hostdata = shost_priv(instance); + hostdata->board = board; hostdata->io = iomem; hostdata->region_size = region_size; @@ -644,7 +646,12 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata, /* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */ if (transfersize % 128) - transfersize = 0; + return 0; + + /* Limit PDMA send to 512 B to avoid random corruption on DTC3181E */ + if (hostdata->board == BOARD_DTC3181E && + cmd->sc_data_direction == DMA_TO_DEVICE) + transfersize = min(cmd->SCp.this_residual, 512); return min(transfersize, DMA_MAX_SIZE); } -- 2.13.0
[PATCH v4 5/5] g_NCR5380: Re-work PDMA loops
From: Ondrej Zary
The polling loops in pread() and pwrite() can easily become infinite
loops and hang the machine.
On DTC chips, IRQ can arrive late and we miss it because we only check
once. Merge the IRQ check into host buffer wait and add polling limit.
Also place a limit on polling for 53C80 registers accessibility.
[Use NCR5380_poll_politely2() for register polling. Rely on polling for
gated IRQ rather than polling for phase error, like the algorithm in the
datasheet. Calculate residual from block count register instead of the
loop counter. Factor-out common code as wait_for_53c80_access(). -- F.T.]
Signed-off-by: Ondrej Zary
Signed-off-by: Finn Thain
---
drivers/scsi/g_NCR5380.c | 147 ++-
1 file changed, 69 insertions(+), 78 deletions(-)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index b1e0a08e49c1..a9e050b65d5f 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -482,6 +482,30 @@ static void generic_NCR5380_release_resources(struct
Scsi_Host *instance)
release_mem_region(base, region_size);
}
+/* wait_for_53c80_access - wait for 53C80 registers to become accessible
+ * @hostdata: scsi host private data
+ *
+ * The registers within the 53C80 logic block are inaccessible until
+ * bit 7 in the 53C400 control status register gets asserted.
+ */
+
+static void wait_for_53c80_access(struct NCR5380_hostdata *hostdata)
+{
+ int count = 1;
+
+ do {
+ if (hostdata->board == BOARD_DTC3181E)
+ udelay(4); /* DTC436 chip hangs without this */
+ if (NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)
+ return;
+ } while (--count > 0);
+
+ scmd_printk(KERN_ERR, hostdata->connected,
+ "53c80 registers not accessible, device will be reset\n");
+ NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
+ NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+}
+
/**
* generic_NCR5380_pread - pseudo DMA receive
* @hostdata: scsi host private data
@@ -494,18 +518,18 @@ static void generic_NCR5380_release_resources(struct
Scsi_Host *instance)
static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
unsigned char *dst, int len)
{
- int blocks = len / 128;
int start = 0;
NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
- NCR5380_write(hostdata->c400_blk_cnt, blocks);
- while (1) {
- if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
+ NCR5380_write(hostdata->c400_blk_cnt, len / 128);
+
+ while (start < len) {
+ if (NCR5380_poll_politely2(hostdata, hostdata->c400_ctl_status,
+ CSR_HOST_BUF_NOT_RDY, 0,
+ hostdata->c400_ctl_status,
+ CSR_GATED_53C80_IRQ,
+ CSR_GATED_53C80_IRQ, HZ / 64) < 0)
break;
- if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ)
- goto out_wait;
- while (NCR5380_read(hostdata->c400_ctl_status) &
CSR_HOST_BUF_NOT_RDY)
- ; /* FIXME - no timeout */
if (hostdata->io_port && hostdata->io_width == 2)
insw(hostdata->io_port + hostdata->c400_host_buf,
@@ -518,38 +542,19 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
hostdata->io + NCR53C400_host_buffer, 128);
start += 128;
- blocks--;
}
- if (blocks) {
- while (NCR5380_read(hostdata->c400_ctl_status) &
CSR_HOST_BUF_NOT_RDY)
- ; /* FIXME - no timeout */
-
- if (hostdata->io_port && hostdata->io_width == 2)
- insw(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 64);
- else if (hostdata->io_port)
- insb(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 128);
- else
- memcpy_fromio(dst + start,
- hostdata->io + NCR53C400_host_buffer, 128);
-
- start += 128;
- blocks--;
- }
-
- if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
- printk("53C400r: no 53C80 gated irq after transfer");
-
-out_wait:
- hostdata->pdma_residual = len - start;
+ hostdata->pdma_residual = NCR5380_read(hostdata->c400_blk_cnt) * 128;
- /* wait for 53C80 registers to be available */
- while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
- ;
+ if
[PATCH v4 3/5] g_NCR5380: Cleanup comments and whitespace
Signed-off-by: Finn Thain
---
drivers/scsi/g_NCR5380.c | 61 ++--
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 911a4300ea51..9f18082415c4 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -1,17 +1,17 @@
/*
* Generic Generic NCR5380 driver
- *
+ *
* Copyright 1993, Drew Eckhardt
- * Visionary Computing
- * (Unix and Linux consulting and custom programming)
- * d...@colorado.edu
- * +1 (303) 440-4894
+ * Visionary Computing
+ * (Unix and Linux consulting and custom programming)
+ * d...@colorado.edu
+ * +1 (303) 440-4894
*
* NCR53C400 extensions (c) 1994,1995,1996, Kevin Lentin
- *k.len...@cs.monash.edu.au
+ * k.len...@cs.monash.edu.au
*
* NCR53C400A extensions (c) 1996, Ingmar Baumgart
- *ing...@gonzo.schwaben.de
+ * ing...@gonzo.schwaben.de
*
* DTC3181E extensions (c) 1997, Ronald van Cuijlenborg
* ronald.van.cuijlenb...@tip.nl or nu...@dds.nl
@@ -481,15 +481,14 @@ static void generic_NCR5380_release_resources(struct
Scsi_Host *instance)
}
/**
- * generic_NCR5380_pread - pseudo DMA read
- * @hostdata: scsi host private data
- * @dst: buffer to read into
- * @len: buffer length
+ * generic_NCR5380_pread - pseudo DMA receive
+ * @hostdata: scsi host private data
+ * @dst: buffer to write into
+ * @len: transfer size
*
- * Perform a pseudo DMA mode read from an NCR53C400 or equivalent
- * controller
+ * Perform a pseudo DMA mode receive from a 53C400 or equivalent device.
*/
-
+
static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
unsigned char *dst, int len)
{
@@ -508,10 +507,10 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
if (hostdata->io_port && hostdata->io_width == 2)
insw(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 64);
+dst + start, 64);
else if (hostdata->io_port)
insb(hostdata->io_port + hostdata->c400_host_buf,
- dst + start, 128);
+dst + start, 128);
else
memcpy_fromio(dst + start,
hostdata->io + NCR53C400_host_buffer, 128);
@@ -558,13 +557,12 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
}
/**
- * generic_NCR5380_pwrite - pseudo DMA write
- * @hostdata: scsi host private data
- * @dst: buffer to read into
- * @len: buffer length
+ * generic_NCR5380_pwrite - pseudo DMA send
+ * @hostdata: scsi host private data
+ * @src: buffer to read from
+ * @len: transfer size
*
- * Perform a pseudo DMA mode read from an NCR53C400 or equivalent
- * controller
+ * Perform a pseudo DMA mode send to a 53C400 or equivalent device.
*/
static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
@@ -603,10 +601,10 @@ static inline int generic_NCR5380_pwrite(struct
NCR5380_hostdata *hostdata,
if (hostdata->io_port && hostdata->io_width == 2)
outsw(hostdata->io_port + hostdata->c400_host_buf,
- src + start, 64);
+ src + start, 64);
else if (hostdata->io_port)
outsb(hostdata->io_port + hostdata->c400_host_buf,
- src + start, 128);
+ src + start, 128);
else
memcpy_toio(hostdata->io + NCR53C400_host_buffer,
src + start, 128);
@@ -656,10 +654,8 @@ static int generic_NCR5380_dma_residual(struct
NCR5380_hostdata *hostdata)
return hostdata->pdma_residual;
}
-/*
- * Include the NCR5380 core code that we build our driver around
- */
-
+/* Include the core driver code. */
+
#include "NCR5380.c"
static struct scsi_host_template driver_template = {
@@ -679,11 +675,10 @@ static struct scsi_host_template driver_template = {
.max_sectors= 128,
};
-
static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
{
int ret = generic_NCR5380_init_one(&driver_template, pdev, base[ndev],
- irq[ndev], card[ndev]);
+ irq[ndev], card[ndev]);
if (ret) {
if (base[ndev])
printk(KERN_WARNING "Card not found at address
0x%03x\n",
@@ -695,7 +690,7 @@ static int generic_NCR5380_isa_match(struct device *pdev,
unsigned int ndev)
}
static int generic_NCR5380_isa_remove(struct device *pdev,
-
[PATCH v4 2/5] g_NCR5380: End PDMA transfer correctly on target disconnection
From: Ondrej Zary
When an IRQ arrives during PDMA transfer, pread() and pwrite() return
without waiting for the 53C80 registers to be ready and this ends up
messing up the chip state. This was observed with SONY CDU-55S which is
slow enough to disconnect during 4096-byte reads.
IRQ during PDMA is not an error so don't return -1. Instead, store the
remaining byte count for use by NCR5380_dma_residual().
[Poll for the BASR_END_DMA_TRANSFER condition rather than remove the
error message -- F.T.]
Signed-off-by: Ondrej Zary
Signed-off-by: Finn Thain
---
drivers/scsi/g_NCR5380.c | 48 +++-
1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 14ef4e8c4713..911a4300ea51 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -44,12 +44,13 @@
int c400_ctl_status; \
int c400_blk_cnt; \
int c400_host_buf; \
- int io_width
+ int io_width; \
+ int pdma_residual
#define NCR5380_dma_xfer_lengeneric_NCR5380_dma_xfer_len
#define NCR5380_dma_recv_setup generic_NCR5380_pread
#define NCR5380_dma_send_setup generic_NCR5380_pwrite
-#define NCR5380_dma_residualNCR5380_dma_residual_none
+#define NCR5380_dma_residualgeneric_NCR5380_dma_residual
#define NCR5380_intrgeneric_NCR5380_intr
#define NCR5380_queue_command generic_NCR5380_queue_command
@@ -500,10 +501,8 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
while (1) {
if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
break;
- if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ) {
- printk(KERN_ERR "53C400r: Got 53C80_IRQ start=%d,
blocks=%d\n", start, blocks);
- return -1;
- }
+ if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ)
+ goto out_wait;
while (NCR5380_read(hostdata->c400_ctl_status) &
CSR_HOST_BUF_NOT_RDY)
; /* FIXME - no timeout */
@@ -542,13 +541,19 @@ static inline int generic_NCR5380_pread(struct
NCR5380_hostdata *hostdata,
if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
printk("53C400r: no 53C80 gated irq after transfer");
+out_wait:
+ hostdata->pdma_residual = len - start;
+
/* wait for 53C80 registers to be available */
while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
;
- if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
- printk(KERN_ERR "53C400r: no end dma signal\n");
-
+ if (NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
+ BASR_END_DMA_TRANSFER, BASR_END_DMA_TRANSFER,
+ HZ / 64) < 0)
+ scmd_printk(KERN_ERR, hostdata->connected, "%s: End of DMA
timeout (%d)\n",
+ __func__, hostdata->pdma_residual);
+
return 0;
}
@@ -571,10 +576,8 @@ static inline int generic_NCR5380_pwrite(struct
NCR5380_hostdata *hostdata,
NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
NCR5380_write(hostdata->c400_blk_cnt, blocks);
while (1) {
- if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ) {
- printk(KERN_ERR "53C400w: Got 53C80_IRQ start=%d,
blocks=%d\n", start, blocks);
- return -1;
- }
+ if (NCR5380_read(hostdata->c400_ctl_status) &
CSR_GATED_53C80_IRQ)
+ goto out_wait;
if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
break;
@@ -612,18 +615,24 @@ static inline int generic_NCR5380_pwrite(struct
NCR5380_hostdata *hostdata,
blocks--;
}
+out_wait:
+ hostdata->pdma_residual = len - start;
+
/* wait for 53C80 registers to be available */
while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)) {
udelay(4); /* DTC436 chip hangs without this */
/* FIXME - no timeout */
}
- if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
- printk(KERN_ERR "53C400w: no end dma signal\n");
- }
-
while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
; // TIMEOUT
+
+ if (NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
+ BASR_END_DMA_TRANSFER, BASR_END_DMA_TRANSFER,
+ HZ / 64) < 0)
+ scmd_printk(KERN_ERR, hostdata->connected, "%s: End of DMA
timeout (%d)\n",
+ __func__, hostdata->pdma_residual);
+
return 0;
}
@@ -
[PATCH v4 0/5] g_NCR5380: PDMA fixes and cleanup
Ondrej, would you please test this new series? Changed since v1: - PDMA transfer residual is calculated earlier. - End of DMA flag check is now polled (if there is any residual). Changed since v2: - Bail out of transfer loops when Gated IRQ gets asserted. - Make udelay conditional on board type. - Drop sg_tablesize patch due to performance regression. Changed since v3: - Add Ondrej's workaround for corrupt WRITE commands on DTC boards. - Reset the 53c400 logic after any short PDMA transfer. - Don't fail the transfer if the 53c400 logic got a reset. Finn Thain (1): g_NCR5380: Cleanup comments and whitespace Ondrej Zary (4): g_NCR5380: Fix PDMA transfer size g_NCR5380: End PDMA transfer correctly on target disconnection g_NCR5380: Limit PDMA send to 512 B to avoid random corruption on DTC3181E g_NCR5380: Re-work PDMA loops drivers/scsi/g_NCR5380.c | 239 --- 1 file changed, 120 insertions(+), 119 deletions(-) -- 2.13.0
[PATCH v4 1/5] g_NCR5380: Fix PDMA transfer size
From: Ondrej Zary
generic_NCR5380_dma_xfer_len() incorrectly uses cmd->transfersize
which causes rescan-scsi-bus and CD-ROM access to hang the system.
Use cmd->SCp.this_residual instead, like other NCR5380 drivers.
Signed-off-by: Ondrej Zary
Signed-off-by: Finn Thain
---
drivers/scsi/g_NCR5380.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 67c8dac321ad..14ef4e8c4713 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -76,6 +76,7 @@
#define IRQ_AUTO 254
#define MAX_CARDS 8
+#define DMA_MAX_SIZE 32768
/* old-style parameters for compatibility */
static int ncr_irq = -1;
@@ -629,23 +630,16 @@ static inline int generic_NCR5380_pwrite(struct
NCR5380_hostdata *hostdata,
static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
struct scsi_cmnd *cmd)
{
- int transfersize = cmd->transfersize;
+ int transfersize = cmd->SCp.this_residual;
if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
return 0;
- /* Limit transfers to 32K, for xx400 & xx406
-* pseudoDMA that transfers in 128 bytes blocks.
-*/
- if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
- !(cmd->SCp.this_residual % transfersize))
- transfersize = 32 * 1024;
-
/* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
if (transfersize % 128)
transfersize = 0;
- return transfersize;
+ return min(transfersize, DMA_MAX_SIZE);
}
/*
--
2.13.0
Re: [PATCH] ses: do not add a device to an enclosure if enclosure_add_links() fails.
On Tue, 2017-06-27 at 21:10 -0400, Martin K. Petersen wrote: > Maurizio, > > > > > The enclosure_add_device() function should fail if it can't > > create the relevant sysfs links. > > James? It's essentially the patch I proposed, so I'm fine with it. Although I would like more root cause analysis about why we have this problem. James
Re: [PATCH v2 0/6] qla2xxx: Add NVMe FC Fabric support in driver
Himanshu, > This patch series adds NVMe FC fabric support for qla2xxx initiator > mode driver. Applied to 4.13/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH RESEND] scsi: sun_esp: fix device reference leaks
Johan, > Make sure to drop the reference to the dma device taken by > of_find_device_by_node() on probe errors and on driver unbind. Looks good to me. Applied to 4.13/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] qla2xxx: Protect access to qpair members with qpair->qp_lock
Hi Martin, On 6/27/17, 6:32 PM, "Martin K. Petersen" wrote: > In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > qpair->qp_lock but do access members of the qpair before having the lock. > Re-order the locking sequence to have all read and write access to qpair > members under the qpair->qp_lock. Cavium folks, please review! I am testing it internally, will update by tomorrow. -- Martin K. Petersen Oracle Linux Engineering Thanks, Himanshu
Re: [PATCH 1/1] fnic: Zero io_cmpl_skip on fw reset completion
Satish, > io_cmpl_skip keep track of number of completions to skip when stats > are reset. If a fw_reset happens immediately after stats reset it > could put it out of sync so need to reset io_cmpl_skip when fw reset > is completed. Applied patches 1-4 to 4.13/scsi-queue. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 0/7] bnx2fc: Update driver to version 2.11.8.
Chad, > Please apply the following patches at your earliest convenience. Applied to 4.13/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: ufs: set host_byte to DID_REQUEUE when ocs = OCS_ABORTED
> Host set ocs to OCS_ABORTED when clear a doorbell in err handler. > Then scsi_decide_disposition return SUCCESS. This may cause some > filesystem panic because a FAILED REQUESET. Requeue and complete is > better. Subhash: Please review the three ufs patches from Zang. https://patchwork.kernel.org/patch/9807763/ https://patchwork.kernel.org/patch/9807759/ https://patchwork.kernel.org/patch/9807755/ -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] qla2xxx: Protect access to qpair members with qpair->qp_lock
> In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > qpair->qp_lock but do access members of the qpair before having the lock. > Re-order the locking sequence to have all read and write access to qpair > members under the qpair->qp_lock. Cavium folks, please review! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: sas: scsi_queue_work can fail, so make callers aware
Johannes, > libsas uses scsi_queue_work() to queue it's internal event > notifications. scsi_queue_work() can return -EINVAL if the work queue > doesn't exist and it does call queue_work() which can return false if > the work is already queued. Applied to 4.13/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] ses: do not add a device to an enclosure if enclosure_add_links() fails.
Maurizio, > The enclosure_add_device() function should fail if it can't > create the relevant sysfs links. James? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH RESEND] scsi: Add STARGET_CREATED_REMOVE state to scsi_target_state
Ewan, > The addition of the STARGET_REMOVE state had the side effect of > introducing a race condition that can cause a crash. > > scsi_target_reap_ref_release() checks the starget->state to > see if it still in STARGET_CREATED, and if so, skips calling > transport_remove_device() and device_del(), because the starget->state > is only set to STARGET_RUNNING after scsi_target_add() has called > device_add() and transport_add_device(). > > However, if an rport loss occurs while a target is being scanned, > it can happen that scsi_remove_target() will be called while the > starget is still in the STARGET_CREATED state. In this case, the > starget->state will be set to STARGET_REMOVE, and as a result, > scsi_target_reap_ref_release() will take the wrong path. The end > result is a panic: Johannes/Bart/James: Please review! -- Martin K. Petersen Oracle Linux Engineering
Re: Permanently change thin provisioning method from user space?
Ewan, > sd_fops->revalidate_disk() will cause the properties that cause the > provisioning_mode to be evaluated to be re-read, and sd_config_discard() > to set the determined mode. We might want to re-think this, since the > user overrode what was probed earlier. However, we might also want to > automatically handle the storage capabilities changing, so I'm not > sure. The intent was for people to use udev to override things. But I guess we could entertain introducing a flag to distinguish between detected and configured state. > My reading of SBC-4r13 6.6.4 is that a WRITE SAME(16) w/UNMAP has > a length limited by the MAXIMUM WRITE SAME LENGTH, and that is what > sd.c implements, but I'm suspicious that the array treated a WRITE SAME(16) > w/UNMAP of 2097152 blocks as an UNMAP and failed it w/ILLEGAL COMMAND, > INVALID FIELD IN CDB. Ugh :( -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
Tejun, >> Looks good to me. I'll queue it up for 4.13 as soon as Linus has pulled >> in the ata bits. > > I can route it through libata tree w/ your ack if that's more convenient. I don't think it will cause any headaches. I'm just a bit cautious since I already have a ton of conflicts in linux-next for this merge window. Acked-by: Martin K. Petersen -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 29/35] bnx2fc: remove obsolete bnx2fc_eh_host_reset() definition
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 23/35] qedf: drop bus reset handler
This is a clear change of behavior, so we should run it past the maintainer. The old behavior looks wrong, but it's probably papering over something else..
Re: [PATCH 18/35] qlogicfas: move bus_reset to host_reset
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 16/35] acornscsi: move bus reset to host reset
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 15/35] qlogicpti: move bus reset to host reset
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 13/35] scsi: drop bus reset for wd33c93-compatible boards
On Fri, Jun 23, 2017 at 03:02:33PM +0200, Hannes Reinecke wrote: > The bus reset function is just a wrapper around host reset, so drop it. It also takes the host_lock, which the direct host_reset invokation doesn't. Given that before we'd always get the bus reset first that probably was the actually tested path, so we should move the lock into wd33c93_host_reset.
Re: [PATCH 14/35] rtsx: drop bus reset function
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 11/35] hptiop: Simplify reset handling
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 09/35] libsas: move bus_reset_handler() to target_reset_handler()
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 10/35] bfa: move bus reset to target reset
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 08/35] lpfc: drop lpfc_no_handler and lpfc_bus_reset_handler()
On Fri, Jun 23, 2017 at 03:02:28PM +0200, Hannes Reinecke wrote:
> lpfc_bus_reset_handler() is really just a loop calling
> lpfc_target_reset_handler() over all targets, which is what
> the error handler will be doing anyway.
> And lpfc_no_handler() will return FAILED, just like SCSI EH
> does if no handler is present.
Looks sensible, but I'd really like to see James looking over this..
>
> So drop both.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/lpfc/lpfc_scsi.c | 99
> ---
> 1 file changed, 99 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index cfe1d01..19b1202 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -5294,93 +5294,6 @@ void lpfc_poll_timeout(unsigned long ptr)
> }
>
> /**
> - * lpfc_bus_reset_handler - scsi_host_template eh_bus_reset_handler entry
> point
> - * @cmnd: Pointer to scsi_cmnd data structure.
> - *
> - * This routine does target reset to all targets on @cmnd->device->host.
> - * This emulates Parallel SCSI Bus Reset Semantics.
> - *
> - * Return code :
> - * 0x2003 - Error
> - * 0x2002 - Success
> - **/
> -static int
> -lpfc_bus_reset_handler(struct scsi_cmnd *cmnd)
> -{
> - struct Scsi_Host *shost = cmnd->device->host;
> - struct lpfc_vport *vport = (struct lpfc_vport *) shost->hostdata;
> - struct lpfc_nodelist *ndlp = NULL;
> - struct lpfc_scsi_event_header scsi_event;
> - int match;
> - int ret = SUCCESS, status, i;
> -
> - scsi_event.event_type = FC_REG_SCSI_EVENT;
> - scsi_event.subcategory = LPFC_EVENT_BUSRESET;
> - scsi_event.lun = 0;
> - memcpy(scsi_event.wwpn, &vport->fc_portname, sizeof(struct lpfc_name));
> - memcpy(scsi_event.wwnn, &vport->fc_nodename, sizeof(struct lpfc_name));
> -
> - fc_host_post_vendor_event(shost, fc_get_event_number(),
> - sizeof(scsi_event), (char *)&scsi_event, LPFC_NL_VENDOR_ID);
> -
> - status = fc_block_scsi_eh(cmnd);
> - if (status != 0 && status != SUCCESS)
> - return status;
> -
> - /*
> - * Since the driver manages a single bus device, reset all
> - * targets known to the driver. Should any target reset
> - * fail, this routine returns failure to the midlayer.
> - */
> - for (i = 0; i < LPFC_MAX_TARGET; i++) {
> - /* Search for mapped node by target ID */
> - match = 0;
> - spin_lock_irq(shost->host_lock);
> - list_for_each_entry(ndlp, &vport->fc_nodes, nlp_listp) {
> - if (!NLP_CHK_NODE_ACT(ndlp))
> - continue;
> - if (vport->phba->cfg_fcp2_no_tgt_reset &&
> - (ndlp->nlp_fcp_info & NLP_FCP_2_DEVICE))
> - continue;
> - if (ndlp->nlp_state == NLP_STE_MAPPED_NODE &&
> - ndlp->nlp_sid == i &&
> - ndlp->rport &&
> - ndlp->nlp_type & NLP_FCP_TARGET) {
> - match = 1;
> - break;
> - }
> - }
> - spin_unlock_irq(shost->host_lock);
> - if (!match)
> - continue;
> -
> - status = lpfc_send_taskmgmt(vport, cmnd,
> - i, 0, FCP_TARGET_RESET);
> -
> - if (status != SUCCESS) {
> - lpfc_printf_vlog(vport, KERN_ERR, LOG_FCP,
> - "0700 Bus Reset on target %d failed\n",
> - i);
> - ret = FAILED;
> - }
> - }
> - /*
> - * We have to clean up i/o as : they may be orphaned by the TMFs
> - * above; or if any of the TMFs failed, they may be in an
> - * indeterminate state.
> - * We will report success if all the i/o aborts successfully.
> - */
> -
> - status = lpfc_reset_flush_io_context(vport, 0, 0, LPFC_CTX_HOST);
> - if (status != SUCCESS)
> - ret = FAILED;
> -
> - lpfc_printf_vlog(vport, KERN_ERR, LOG_FCP,
> - "0714 SCSI layer issued Bus Reset Data: x%x\n", ret);
> - return ret;
> -}
> -
> -/**
> * lpfc_host_reset_handler - scsi_host_template eh_host_reset_handler entry
> pt
> * @cmnd: Pointer to scsi_cmnd data structure.
> *
> @@ -5926,11 +5839,6 @@ struct lpfc_device_data*
> return SCSI_MLQUEUE_HOST_BUSY;
> }
>
> -static int
> -lpfc_no_handler(struct scsi_cmnd *cmnd)
> -{
> - return FAILED;
> -}
>
> static int
> lpfc_no_slave(struct scsi_device *sdev)
> @@ -5944,11 +5852,6 @@ struct scsi_host_template lpfc_template_nvme = {
> .proc_name = LPFC_DRIVER_NAME,
> .info = lpfc_info,
> .queuecommand = lpfc_no_command,
> - .eh_abort_handler = lpfc_no_handler,
> - .
Re: [PATCH 06/35] visorhba: sanitze private device data allocation
On Fri, Jun 23, 2017 at 03:02:26PM +0200, Hannes Reinecke wrote: > There's no need to keep the private data for a device in a separate > list; better to store it in ->hostdata and do away with the additional > list. Nice improvement, although that driver is a complete mess.. And I guess you need to send this to Greg separately.
Re: [PATCH 05/35] fnic: do not call host reset from command abort
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 07/35] uas: move eh_bus_reset_handler to eh_device_reset_handler
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 01/35] scsi: fix comment in scsi_device_set_state()
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 03/35] ibmvfc: Do not call fc_block_scsi_eh() on host reset
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 02/35] mptfc: Do not call fc_block_scsi_eh() on host reset
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 04/35] fc_fcp: do not call fc_block_scsi_eh() from host reset
Loks fine, Reviewed-by: Christoph Hellwig
Re: [RFC PATCH 00/35] SCSI EH argument reshuffling
Can you send a series with just the prep patches in the beginning? I think some of this could be 4.13 material still, leaving the actual prototype changes for 4.14.
[PATCH] scsi: osst: remove useless variable assignments in osst_int_ioctl()
Value assigned to variable blkno at lines
4123:if (blkno >= 0) blkno += arg; and
4127:if (blkno >= 0) blkno -= arg;
is overwritten at line 4131:blkno = STps->drv_block;
before it can be used.
This makes such variable assignments useless.
Addresses-Coverity-ID: 1397685
Signed-off-by: Gustavo A. R. Silva
---
drivers/scsi/osst.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 929ee7e..a62c1f2 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -4118,14 +4118,11 @@ static int osst_int_ioctl(struct osst_tape * STp,
struct osst_request ** aSRpnt,
printk(OSST_DEB_MSG "%s:D: Skipping %lu blocks %s from
logical block %d\n",
name, arg, cmd_in==MTFSR?"forward":"backward",
logical_blk_num);
#endif
- if (cmd_in == MTFSR) {
- logical_blk_num += arg;
- if (blkno >= 0) blkno += arg;
- }
- else {
- logical_blk_num -= arg;
- if (blkno >= 0) blkno -= arg;
- }
+ if (cmd_in == MTFSR)
+ logical_blk_num += arg;
+ else
+ logical_blk_num -= arg;
+
ioctl_result = osst_seek_logical_blk(STp, &SRpnt,
logical_blk_num);
fileno = STps->drv_file;
blkno = STps->drv_block;
--
2.5.0
Re: [PATCH 0/5] v3 block subsystem refcounter conversions
On Tue, Jun 27, 2017 at 6:26 AM, Jens Axboe wrote:
> On 06/27/2017 05:39 AM, Elena Reshetova wrote:
>> Changes in v3:
>> No changes in patches apart from trivial rebases, but now by
>> default refcount_t = atomic_t and uses all atomic standard operations
>> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the
>> systems that are critical on performance and cannot accept even
>> slight delay on the refcounter operations.
>
> Is that true in 4.12-rc, or is that true in a later release once
> Linus has pulled those changes in? If the latter, please resend
> this when those changes are in, thanks.
It's in -next currently ("locking/refcount: Create unchecked atomic_t
implementation")
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH RESEND] scsi: Add STARGET_CREATED_REMOVE state to scsi_target_state
On 06/27/2017 02:55 PM, Ewan D. Milne wrote:
From: "Ewan D. Milne"
The addition of the STARGET_REMOVE state had the side effect of
introducing a race condition that can cause a crash.
scsi_target_reap_ref_release() checks the starget->state to
see if it still in STARGET_CREATED, and if so, skips calling
transport_remove_device() and device_del(), because the starget->state
is only set to STARGET_RUNNING after scsi_target_add() has called
device_add() and transport_add_device().
However, if an rport loss occurs while a target is being scanned,
it can happen that scsi_remove_target() will be called while the
starget is still in the STARGET_CREATED state. In this case, the
starget->state will be set to STARGET_REMOVE, and as a result,
scsi_target_reap_ref_release() will take the wrong path. The end
result is a panic:
[ 1255.356653] Oops: [#1] SMP
[ 1255.360154] Modules linked in: x86_pkg_temp_thermal kvm_intel kvm irqbypass
crc32c_intel ghash_clmulni_i
[ 1255.393234] CPU: 5 PID: 149 Comm: kworker/u96:4 Tainted: GW
4.11.0+ #8
[ 1255.401879] Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22
11/19/2013
[ 1255.410327] Workqueue: scsi_wq_6 fc_scsi_scan_rport [scsi_transport_fc]
[ 1255.417720] task: 88060ca8c8c0 task.stack: c900048a8000
[ 1255.424331] RIP: 0010:kernfs_find_ns+0x13/0xc0
[ 1255.429287] RSP: 0018:c900048abbf0 EFLAGS: 00010246
[ 1255.435123] RAX: RBX: RCX:
[ 1255.443083] RDX: RSI: 8188d659 RDI:
[ 1255.451043] RBP: c900048abc10 R08: R09: 012433fe0025
[ 1255.459005] R10: 25e5a4b5 R11: 25e5a4b5 R12: 8188d659
[ 1255.466972] R13: R14: 8805f55e5088 R15:
[ 1255.474931] FS: () GS:880616b4()
knlGS:
[ 1255.483959] CS: 0010 DS: ES: CR0: 80050033
[ 1255.490370] CR2: 0068 CR3: 01c09000 CR4: 000406e0
[ 1255.498332] Call Trace:
[ 1255.501058] kernfs_find_and_get_ns+0x31/0x60
[ 1255.505916] sysfs_unmerge_group+0x1d/0x60
[ 1255.510498] dpm_sysfs_remove+0x22/0x60
[ 1255.514783] device_del+0xf4/0x2e0
[ 1255.518577] ? device_remove_file+0x19/0x20
[ 1255.523241] attribute_container_class_device_del+0x1a/0x20
[ 1255.529457] transport_remove_classdev+0x4e/0x60
[ 1255.534607] ? transport_add_class_device+0x40/0x40
[ 1255.540046] attribute_container_device_trigger+0xb0/0xc0
[ 1255.546069] transport_remove_device+0x15/0x20
[ 1255.551025] scsi_target_reap_ref_release+0x25/0x40
[ 1255.556467] scsi_target_reap+0x2e/0x40
[ 1255.560744] __scsi_scan_target+0xaa/0x5b0
[ 1255.565312] scsi_scan_target+0xec/0x100
[ 1255.569689] fc_scsi_scan_rport+0xb1/0xc0 [scsi_transport_fc]
[ 1255.576099] process_one_work+0x14b/0x390
[ 1255.580569] worker_thread+0x4b/0x390
[ 1255.584651] kthread+0x109/0x140
[ 1255.588251] ? rescuer_thread+0x330/0x330
[ 1255.592730] ? kthread_park+0x60/0x60
[ 1255.596815] ret_from_fork+0x29/0x40
[ 1255.600801] Code: 24 08 48 83 42 40 01 5b 41 5c 5d c3 66 66 66 2e 0f 1f 84
00 00 00 00 00 66 66 66 66 90
[ 1255.621876] RIP: kernfs_find_ns+0x13/0xc0 RSP: c900048abbf0
[ 1255.628479] CR2: 0068
[ 1255.632756] ---[ end trace 34a69ba0477d036f ]---
Fix this by adding another scsi_target state STARGET_CREATED_REMOVE
to distinguish this case.
Fixes: f05795d3d771 ("scsi: Add intermediate STARGET_REMOVE state to
scsi_target_state")
Reported-by: David Jeffery
Signed-off-by: Ewan D. Milne
Cc: sta...@vger.kernel.org
---
drivers/scsi/scsi_scan.c | 5 +++--
drivers/scsi/scsi_sysfs.c | 8 ++--
include/scsi/scsi_device.h | 1 +
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3c44032..3832ba5 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -385,11 +385,12 @@ static void scsi_target_reap_ref_release(struct kref
*kref)
= container_of(kref, struct scsi_target, reap_ref);
/*
-* if we get here and the target is still in the CREATED state that
+* if we get here and the target is still in a CREATED state that
* means it was allocated but never made visible (because a scan
* turned up no LUNs), so don't call device_del() on it.
*/
- if (starget->state != STARGET_CREATED) {
+ if ((starget->state != STARGET_CREATED) &&
+ (starget->state != STARGET_CREATED_REMOVE)) {
transport_remove_device(&starget->dev);
device_del(&starget->dev);
}
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ce470f6..d6984df 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1394,11 +1394,15 @@ void scsi_remove_target(struct device *dev)
spin_lock_irqsave(shost->host_lock, flags);
list_for_each_entry(starget, &sh
Re: Permanently change thin provisioning method from user space?
On Tue, 2017-06-27 at 14:02 +0300, Andrei Borzenkov wrote: > I'm facing storage system that while generally advertising support for > WRITE_SAME_16 with UNMAP in reality fails this request depending on > exact volume configuration. This configuration is done on storage side > and may happen after host initially discovered LUN. > > I though about simply overriding kernel auto-detect with UNMAP using > sysfs scsi_disk/.../provisioning_mode attribute, but it appears kernel > may re-evaluate it in some cases, so changes do not stick. Is it > possible to permanently restrict it per-device (or per-vendor/device)? > > TIA > > -andrei sd_fops->revalidate_disk() will cause the properties that cause the provisioning_mode to be evaluated to be re-read, and sd_config_discard() to set the determined mode. We might want to re-think this, since the user overrode what was probed earlier. However, we might also want to automatically handle the storage capabilities changing, so I'm not sure. I think we should figure out how best to handle devices/arrays that aren't correctly handling WRITE SAME(16) w/UNMAP, I've seen a few cases of this recently with a certain FC array and it looks to me as if the problem there was that it was advertising MAXIMUM WRITE SAME LENGTH of zero in the block limits VPD page, but a MAXIMUM UNMAP LBA COUNT of 65536. My reading of SBC-4r13 6.6.4 is that a WRITE SAME(16) w/UNMAP has a length limited by the MAXIMUM WRITE SAME LENGTH, and that is what sd.c implements, but I'm suspicious that the array treated a WRITE SAME(16) w/UNMAP of 2097152 blocks as an UNMAP and failed it w/ILLEGAL COMMAND, INVALID FIELD IN CDB. I don't actually have one of these arrays, I'm trying to gather more info. -Ewan
[PATCH RESEND] scsi: Add STARGET_CREATED_REMOVE state to scsi_target_state
From: "Ewan D. Milne"
The addition of the STARGET_REMOVE state had the side effect of
introducing a race condition that can cause a crash.
scsi_target_reap_ref_release() checks the starget->state to
see if it still in STARGET_CREATED, and if so, skips calling
transport_remove_device() and device_del(), because the starget->state
is only set to STARGET_RUNNING after scsi_target_add() has called
device_add() and transport_add_device().
However, if an rport loss occurs while a target is being scanned,
it can happen that scsi_remove_target() will be called while the
starget is still in the STARGET_CREATED state. In this case, the
starget->state will be set to STARGET_REMOVE, and as a result,
scsi_target_reap_ref_release() will take the wrong path. The end
result is a panic:
[ 1255.356653] Oops: [#1] SMP
[ 1255.360154] Modules linked in: x86_pkg_temp_thermal kvm_intel kvm irqbypass
crc32c_intel ghash_clmulni_i
[ 1255.393234] CPU: 5 PID: 149 Comm: kworker/u96:4 Tainted: GW
4.11.0+ #8
[ 1255.401879] Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22
11/19/2013
[ 1255.410327] Workqueue: scsi_wq_6 fc_scsi_scan_rport [scsi_transport_fc]
[ 1255.417720] task: 88060ca8c8c0 task.stack: c900048a8000
[ 1255.424331] RIP: 0010:kernfs_find_ns+0x13/0xc0
[ 1255.429287] RSP: 0018:c900048abbf0 EFLAGS: 00010246
[ 1255.435123] RAX: RBX: RCX:
[ 1255.443083] RDX: RSI: 8188d659 RDI:
[ 1255.451043] RBP: c900048abc10 R08: R09: 012433fe0025
[ 1255.459005] R10: 25e5a4b5 R11: 25e5a4b5 R12: 8188d659
[ 1255.466972] R13: R14: 8805f55e5088 R15:
[ 1255.474931] FS: () GS:880616b4()
knlGS:
[ 1255.483959] CS: 0010 DS: ES: CR0: 80050033
[ 1255.490370] CR2: 0068 CR3: 01c09000 CR4: 000406e0
[ 1255.498332] Call Trace:
[ 1255.501058] kernfs_find_and_get_ns+0x31/0x60
[ 1255.505916] sysfs_unmerge_group+0x1d/0x60
[ 1255.510498] dpm_sysfs_remove+0x22/0x60
[ 1255.514783] device_del+0xf4/0x2e0
[ 1255.518577] ? device_remove_file+0x19/0x20
[ 1255.523241] attribute_container_class_device_del+0x1a/0x20
[ 1255.529457] transport_remove_classdev+0x4e/0x60
[ 1255.534607] ? transport_add_class_device+0x40/0x40
[ 1255.540046] attribute_container_device_trigger+0xb0/0xc0
[ 1255.546069] transport_remove_device+0x15/0x20
[ 1255.551025] scsi_target_reap_ref_release+0x25/0x40
[ 1255.556467] scsi_target_reap+0x2e/0x40
[ 1255.560744] __scsi_scan_target+0xaa/0x5b0
[ 1255.565312] scsi_scan_target+0xec/0x100
[ 1255.569689] fc_scsi_scan_rport+0xb1/0xc0 [scsi_transport_fc]
[ 1255.576099] process_one_work+0x14b/0x390
[ 1255.580569] worker_thread+0x4b/0x390
[ 1255.584651] kthread+0x109/0x140
[ 1255.588251] ? rescuer_thread+0x330/0x330
[ 1255.592730] ? kthread_park+0x60/0x60
[ 1255.596815] ret_from_fork+0x29/0x40
[ 1255.600801] Code: 24 08 48 83 42 40 01 5b 41 5c 5d c3 66 66 66 2e 0f 1f 84
00 00 00 00 00 66 66 66 66 90
[ 1255.621876] RIP: kernfs_find_ns+0x13/0xc0 RSP: c900048abbf0
[ 1255.628479] CR2: 0068
[ 1255.632756] ---[ end trace 34a69ba0477d036f ]---
Fix this by adding another scsi_target state STARGET_CREATED_REMOVE
to distinguish this case.
Fixes: f05795d3d771 ("scsi: Add intermediate STARGET_REMOVE state to
scsi_target_state")
Reported-by: David Jeffery
Signed-off-by: Ewan D. Milne
Cc: sta...@vger.kernel.org
---
drivers/scsi/scsi_scan.c | 5 +++--
drivers/scsi/scsi_sysfs.c | 8 ++--
include/scsi/scsi_device.h | 1 +
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3c44032..3832ba5 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -385,11 +385,12 @@ static void scsi_target_reap_ref_release(struct kref
*kref)
= container_of(kref, struct scsi_target, reap_ref);
/*
-* if we get here and the target is still in the CREATED state that
+* if we get here and the target is still in a CREATED state that
* means it was allocated but never made visible (because a scan
* turned up no LUNs), so don't call device_del() on it.
*/
- if (starget->state != STARGET_CREATED) {
+ if ((starget->state != STARGET_CREATED) &&
+ (starget->state != STARGET_CREATED_REMOVE)) {
transport_remove_device(&starget->dev);
device_del(&starget->dev);
}
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ce470f6..d6984df 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1394,11 +1394,15 @@ void scsi_remove_target(struct device *dev)
spin_lock_irqsave(shost->host_lock, flags);
list_for_each_entry(starget, &shost->__targets, siblings) {
if (st
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tuesday 27 June 2017 03:49:16 Finn Thain wrote:
> On Mon, 26 Jun 2017, Ondrej Zary wrote:
> > No apparent change in behavior, the first write test resulted in:
> > [ 842.830802] sd 2:0:1:0: [sdb] tag#0 53c80 registers not accessible,
> > device will be reset [ 842.830802] sd 2:0:1:0: [sdb] tag#0 switching to
> > slow handshake
> >
> > Checking for IRQ after poll_politely2 does not seem right because we can
> > miss the buffer ready signal.
>
> How so? As long as there's no gated IRQ, we poll for buffer readiness
> until timeout. And when there is a gated IRQ, we break both the polling
> loop and the transfer loop immediately. Your code and mine are basically
> in agreement here.
Yes, it stops transfer when an IRQ arrives. But the host buffer could be ready
at the same time. The IRQ can be an "end-of-DMA" IRQ (IIRC DTC chips assert
this earlier than 53C400). Or just a disconnect that occured right now but the
chip already read a buffer full of data.
> > According to my tests, buffer ready signal is most important - if there
> > is any data to read/write, do the transfer. If not, only then check why
> > - maybe we got an IRQ (that terminated PDMA). Or no IRQ, sometimes the
> > wait for buffer ready times out - we need to terminate PDMA manually
> > then (reset).
> >
> > Then 53C80 registers should become ready.
>
> You seem to be saying that we should ignore the IRQ signal if the buffers
> have become ready. Maybe so. Can we try simply resetting the block
> counter? (I could imagine that the 53c400 core might leave the 53c80
> registers inaccessible unless we keep accessing the buffers in the 53c400
> core until the transfer is done.)
We can't reset the block counter because 0 means 256 blocks to transfer (page
13 in datasheet).
Yes, the 53C80 registers seem to become available only when the PDMA transfer
ends by either:
1. transferring all blocks (block counter decrementing to zero)
2. IRQ
3. reset
> BTW, with regard to your patch, note that this construct is race prone:
>
> while (1) { /* monitor IRQ while waiting for host buffer */
> csr = NCR5380_read(hostdata->c400_ctl_status);
> if (!(csr & CSR_HOST_BUF_NOT_RDY))
> break;
> if (csr & CSR_GATED_53C80_IRQ) {
> basr = NCR5380_read(BUS_AND_STATUS_REG);
> if (!(basr & BASR_PHASE_MATCH) ||
> (basr & BASR_BUSY_ERROR)) {
> printk("basr=0x%02x csr=0x%02x at start=%d\n", basr,
> csr, start);
> goto out_wait;
> }
> }
> if (retries-- < 1) {
> shost_printk(KERN_ERR, hostdata->host, "53C400r: host buffer
> not ready in
> time\n"); NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
> NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
> goto out_wait;
> }
> }
>
> This code can "goto out_wait" when !(csr & CSR_HOST_BUF_NOT_RDY). It
> depends on timing. This would seem to be contrary to your stated aim.
>
> Moreover, this code can also "break" when (csr & CSR_GATED_53C80_IRQ).
> That depends on timing too. But this may be an improvement on my code if
> it allows the 53c80 registers to become accessible, by allowing the block
> counter to be decremented.
Yes, it continue the transfer even if the IRQ is asserted - as long as the
buffer is ready. That's intended.
> The uncertainty here was one of the reasons I reworked this code.
My version reads CSR only once per loop but that probably does not help at all
as the HW state could change anytime. The chip's design seems to be very
race-prone.
> > This is a log from writing 230 MB file using my code with some debug
> > prints, on a NCR53C400. No 53C80 timeouts, only disconnects and some
> > host buffer timeouts (maybe the drive sometimes just slows down without
> > disconnecting?)
> >
> > [ 3378.503828] basr=0x10 csr=0xd5 at start=512
> > [ 3461.257973] w basr=0x10 csr=0x95 at start=3840
> > [ 3461.838225] w basr=0x10 csr=0x95 at start=3840
> > [ 3462.683446] w basr=0x10 csr=0x95 at start=3840
> > [ 3463.416911] w basr=0x10 csr=0x95 at start=3840
> > [ 3465.117804] scsi host2: 53C400w: host buffer not ready in time
> > [ 3465.276375] w basr=0x10 csr=0x95 at start=3328
> > [ 3466.457701] w basr=0x10 csr=0x95 at start=1792
> > [ 3467.019643] scsi host2: 53C400w: host buffer not ready in time
> > [ 3467.619058] scsi host2: 53C400w: host buffer not ready in time
> > [ 3467.799619] w basr=0x10 csr=0x95 at start=3840
> > [ 3552.123501] w basr=0x10 csr=0x95 at start=2304
> > [ 3552.771223] w basr=0x10 csr=0x95 at start=1280
> > [ 3554.556451] w basr=0x10 csr=0x95 at start=2816
> > [ 3555.229646] w basr=0x10 csr=0x95 at start=1792
> > [ 3555.630632] scsi host2: 53C400w: host buffer not ready in time
> > [ 3555.774560] w basr=0x10 csr=0x95 at start=768
> > [ 3625.541608] w basr=0x10 csr=0x95 at start=3328
> > [ 3640.099861] w basr=0x10 csr=0x95 at start=1792
> > [ 3641.442671] w basr=0x10 csr=0x95 at start=2816
> >
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tuesday 27 June 2017 14:42:29 Finn Thain wrote:
> On Tue, 27 Jun 2017, Ondrej Zary wrote:
> > BTW. I've probably found the DTC write corruption. Added the following
> > check (13 is host buffer index register) -
>
> That register is not mentioned in my 53c400 datasheet.
Yes, it's not there. But we don't have 53C400A and DTC436 datasheets (this
register works on both).
> > and it triggers sometimes: the value is 1 instead of 0. As we use only
> > 16-bit writes, I don't see how the value could ever be odd. Looks like a
> > bug in the chip. The index register corrupts during the transfer, not
> > after IRQ or timeout. The same check at beginning of pwrite() did not
> > trigger.
>
> Are you reading this register at the right moment? Have you tried waiting
> for it to reach zero, as in,
>
> if (NCR5380_poll_politely(hostdata, 13, 0xff, 0, HZ / 64) < 0)
> /* printk, reset etc */;
I have not but will try (expecting that it will not change by itself).
> Even if this is a reliable way to detect a short transfer, it would be
> nice to know the root cause. But I'm being unrealistic: the DTC436 vendor
> never responded to my requests for technical documentation.
According to the data corruption observed, it's not a short transfer. The
corruption is always the same: one byte missing at the beginning of a 128 B
block. It happens only with slow Quantum LPS 240 drive, not with faster IBM
DORS-32160.
> > The index register is not writable so we must(?) reset the PDMA engine
> > to recover. However, this quick attempt to fix does not work, maybe we
> > should reload the block count and continue?
>
> I don't know if it is possible to recover. If the last byte never reached
> the scsi bus, then once you reset the 53c400 core, you need the driver to
> perform a single-byte PIO transfer after the short PDMA transfer. This
> would require that you set the residual appropriately (though in my
> experience that may not be sufficient).
>
> It may be better to simply limit the transfer to 512 bytes instead of
> attempting to recover based on an undocumented (?) register, etc. Seems
> like a bit of a hack.
>
> > --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct
> > NCR5380_hostdata *hostdata,
> > goto out_wait;
> > }
> > }
> > -
> > + idx = NCR5380_read(13);
> > + if (idx != 0) {
> > + printk("host idx=%d, start=%d\n", idx, start);
> > + NCR5380_write(hostdata->c400_ctl_status,
> > CSR_RESET); +
> > NCR5380_write(hostdata->c400_ctl_status, CSR_BASE); +
> > goto out_wait;
> > + }
> > if (hostdata->io_port && hostdata->io_width == 2)
> > outsw(hostdata->io_port +
> > hostdata->c400_host_buf, src + start, 64);
>
> I find it hard to reason about this code. For example, out_wait is to be
> removed. Let's get the preceding patches working and signed-off. Please go
> ahead and use a 512 B transfer for DTC436 testing if that will help get
> this patch series over the line.
OK, I agree. Let's fix the problems first and leave this hack for later.
--
Ondrej Zary
Re: [PATCH] libata: Support for an ATA PASS-THROUGH(32) command.
On Sat, Jun 24, 2017 at 03:41:10AM +0900, Minwoo Im wrote: > SAT-4(SCSI/ATA Translation) supports for an ata pass-thru(32). > This patch will allow to translate an ata pass-thru(32) SCSI cmd > to an ATA cmd. > > Signed-off-by: Minwoo Im > Reviewed-by: Bart Van Assche Applied to libata/for-4.13. Thanks. -- tejun
Re: [PATCH 0/5] v3 block subsystem refcounter conversions
On 06/27/2017 05:39 AM, Elena Reshetova wrote: > Changes in v3: > No changes in patches apart from trivial rebases, but now by > default refcount_t = atomic_t and uses all atomic standard operations > unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the > systems that are critical on performance and cannot accept even > slight delay on the refcounter operations. Is that true in 4.12-rc, or is that true in a later release once Linus has pulled those changes in? If the latter, please resend this when those changes are in, thanks. -- Jens Axboe
Re: [PATCH] ses: do not add a device to an enclosure if enclosure_add_links() fails.
On 06/27/2017 04:53 AM, Maurizio Lombardi wrote:
The enclosure_add_device() function should fail if it can't
create the relevant sysfs links.
Signed-off-by: Maurizio Lombardi
---
drivers/misc/enclosure.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index d3fe3ea..eb29113 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int
component,
struct device *dev)
{
struct enclosure_component *cdev;
+ int err;
if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,17 @@ int enclosure_add_device(struct enclosure_device *edev,
int component,
if (cdev->dev == dev)
return -EEXIST;
- if (cdev->dev)
+ if (cdev->dev) {
enclosure_remove_links(cdev);
-
- put_device(cdev->dev);
+ put_device(cdev->dev);
+ }
cdev->dev = get_device(dev);
- return enclosure_add_links(cdev);
+ err = enclosure_add_links(cdev);
+ if (err) {
+ put_device(cdev->dev);
+ cdev->dev = NULL;
+ }
+ return err;
}
EXPORT_SYMBOL_GPL(enclosure_add_device);
Tested-by: Douglas Miller
This fixes a problem where udevd (insmod ses) races with/overtakes
do_scan_async(), which creates the directory target of the symlink,
resulting in missing enclosure symlinks. This patch relaxes the symlink
creation allowing for delayed addition to enclosure and creation of
symlinks after do_scan_async() has created the target directory.
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tue, 27 Jun 2017, Michael Schmitz wrote: > Ondrej, > > could this be a partial write (target did not transfer the last byte)? > We do wait for TCR_LAST_BYTE_SENT, but only when there is no residual. Perhaps we should wait for TCR_LAST_BYTE_SENT whenever the 53c400 asserts /EOP. That is, whenever BASR_END_DMA_TRANSFER is set. -- > One would suppose the chip posts a phase mismatch in that case ... > > Cheers, > > Michael >
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
On Tue, 27 Jun 2017, Ondrej Zary wrote:
> BTW. I've probably found the DTC write corruption. Added the following
> check (13 is host buffer index register) -
That register is not mentioned in my 53c400 datasheet.
> and it triggers sometimes: the value is 1 instead of 0. As we use only
> 16-bit writes, I don't see how the value could ever be odd. Looks like a
> bug in the chip. The index register corrupts during the transfer, not
> after IRQ or timeout. The same check at beginning of pwrite() did not
> trigger.
>
Are you reading this register at the right moment? Have you tried waiting
for it to reach zero, as in,
if (NCR5380_poll_politely(hostdata, 13, 0xff, 0, HZ / 64) < 0)
/* printk, reset etc */;
Even if this is a reliable way to detect a short transfer, it would be
nice to know the root cause. But I'm being unrealistic: the DTC436 vendor
never responded to my requests for technical documentation.
> The index register is not writable so we must(?) reset the PDMA engine
> to recover. However, this quick attempt to fix does not work, maybe we
> should reload the block count and continue?
>
I don't know if it is possible to recover. If the last byte never reached
the scsi bus, then once you reset the 53c400 core, you need the driver to
perform a single-byte PIO transfer after the short PDMA transfer. This
would require that you set the residual appropriately (though in my
experience that may not be sufficient).
It may be better to simply limit the transfer to 512 bytes instead of
attempting to recover based on an undocumented (?) register, etc. Seems
like a bit of a hack.
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct
> NCR5380_hostdata *hostdata,
> goto out_wait;
> }
> }
> -
> + idx = NCR5380_read(13);
> + if (idx != 0) {
> + printk("host idx=%d, start=%d\n", idx, start);
> + NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
> + NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
> + goto out_wait;
> + }
> if (hostdata->io_port && hostdata->io_width == 2)
> outsw(hostdata->io_port + hostdata->c400_host_buf,
> src + start, 64);
>
I find it hard to reason about this code. For example, out_wait is to be
removed. Let's get the preceding patches working and signed-off. Please go
ahead and use a 512 B transfer for DTC436 testing if that will help get
this patch series over the line.
--
[PATCH 3/5] block: convert blkcg_gq.refcnt from atomic_t to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova
Signed-off-by: Hans Liljestrand
Signed-off-by: Kees Cook
Signed-off-by: David Windsor
---
block/blk-cgroup.c | 2 +-
include/linux/blk-cgroup.h | 9 -
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 0480892..3762908 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -107,7 +107,7 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg,
struct request_queue *q,
blkg->q = q;
INIT_LIST_HEAD(&blkg->q_node);
blkg->blkcg = blkcg;
- atomic_set(&blkg->refcnt, 1);
+ refcount_set(&blkg->refcnt, 1);
/* root blkg uses @q->root_rl, init rl only for !root blkgs */
if (blkcg != &blkcg_root) {
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 01b62e7..e54f048 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
/* percpu_counter batch for blkg_[rw]stats, per-cpu drift doesn't matter */
#define BLKG_STAT_CPU_BATCH(INT_MAX / 2)
@@ -122,7 +123,7 @@ struct blkcg_gq {
struct request_list rl;
/* reference count */
- atomic_trefcnt;
+ refcount_t refcnt;
/* is this blkg online? protected by both blkcg and q locks */
boolonline;
@@ -354,8 +355,7 @@ static inline int blkg_path(struct blkcg_gq *blkg, char
*buf, int buflen)
*/
static inline void blkg_get(struct blkcg_gq *blkg)
{
- WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
- atomic_inc(&blkg->refcnt);
+ refcount_inc(&blkg->refcnt);
}
void __blkg_release_rcu(struct rcu_head *rcu);
@@ -366,8 +366,7 @@ void __blkg_release_rcu(struct rcu_head *rcu);
*/
static inline void blkg_put(struct blkcg_gq *blkg)
{
- WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
- if (atomic_dec_and_test(&blkg->refcnt))
+ if (refcount_dec_and_test(&blkg->refcnt))
call_rcu(&blkg->rcu_head, __blkg_release_rcu);
}
--
2.7.4
[PATCH 4/5] block: convert io_context.active_ref from atomic_t to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova
Signed-off-by: Hans Liljestrand
Signed-off-by: Kees Cook
Signed-off-by: David Windsor
---
block/bfq-iosched.c | 2 +-
block/blk-ioc.c | 4 ++--
block/cfq-iosched.c | 4 ++--
include/linux/iocontext.h | 6 +++---
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index ed93da2..3a525ab 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3988,7 +3988,7 @@ static void bfq_update_idle_window(struct bfq_data *bfqd,
enable_idle = bfq_bfqq_idle_window(bfqq);
- if (atomic_read(&bic->icq.ioc->active_ref) == 0 ||
+ if (refcount_read(&bic->icq.ioc->active_ref) == 0 ||
bfqd->bfq_slice_idle == 0 ||
(bfqd->hw_tag && BFQQ_SEEKY(bfqq) &&
bfqq->wr_coeff == 1))
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 63898d2..69704d2 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -176,7 +176,7 @@ void put_io_context_active(struct io_context *ioc)
unsigned long flags;
struct io_cq *icq;
- if (!atomic_dec_and_test(&ioc->active_ref)) {
+ if (!refcount_dec_and_test(&ioc->active_ref)) {
put_io_context(ioc);
return;
}
@@ -275,7 +275,7 @@ int create_task_io_context(struct task_struct *task, gfp_t
gfp_flags, int node)
/* initialize */
atomic_long_set(&ioc->refcount, 1);
atomic_set(&ioc->nr_tasks, 1);
- atomic_set(&ioc->active_ref, 1);
+ refcount_set(&ioc->active_ref, 1);
spin_lock_init(&ioc->lock);
INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->icq_list);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index b7e9c7f..07c416a 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2973,7 +2973,7 @@ static void cfq_arm_slice_timer(struct cfq_data *cfqd)
* task has exited, don't wait
*/
cic = cfqd->active_cic;
- if (!cic || !atomic_read(&cic->icq.ioc->active_ref))
+ if (!cic || !refcount_read(&cic->icq.ioc->active_ref))
return;
/*
@@ -3965,7 +3965,7 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct
cfq_queue *cfqq,
if (cfqq->next_rq && req_noidle(cfqq->next_rq))
enable_idle = 0;
- else if (!atomic_read(&cic->icq.ioc->active_ref) ||
+ else if (!refcount_read(&cic->icq.ioc->active_ref) ||
!cfqd->cfq_slice_idle ||
(!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
enable_idle = 0;
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index df38db2..e47b907 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -3,6 +3,7 @@
#include
#include
+#include
#include
enum {
@@ -96,7 +97,7 @@ struct io_cq {
*/
struct io_context {
atomic_long_t refcount;
- atomic_t active_ref;
+ refcount_t active_ref;
atomic_t nr_tasks;
/* all the fields below are protected by this lock */
@@ -128,9 +129,8 @@ struct io_context {
static inline void get_io_context_active(struct io_context *ioc)
{
WARN_ON_ONCE(atomic_long_read(&ioc->refcount) <= 0);
- WARN_ON_ONCE(atomic_read(&ioc->active_ref) <= 0);
atomic_long_inc(&ioc->refcount);
- atomic_inc(&ioc->active_ref);
+ refcount_inc(&ioc->active_ref);
}
static inline void ioc_task_link(struct io_context *ioc)
--
2.7.4
[PATCH 5/5] block: convert bsg_device.ref_count from atomic_t to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova
Signed-off-by: Hans Liljestrand
Signed-off-by: Kees Cook
Signed-off-by: David Windsor
---
block/bsg.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index 6fd0854..f35e721 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
@@ -38,7 +39,7 @@ struct bsg_device {
struct list_head busy_list;
struct list_head done_list;
struct hlist_node dev_list;
- atomic_t ref_count;
+ refcount_t ref_count;
int queued_cmds;
int done_cmds;
wait_queue_head_t wq_done;
@@ -711,7 +712,7 @@ static int bsg_put_device(struct bsg_device *bd)
mutex_lock(&bsg_mutex);
- do_free = atomic_dec_and_test(&bd->ref_count);
+ do_free = refcount_dec_and_test(&bd->ref_count);
if (!do_free) {
mutex_unlock(&bsg_mutex);
goto out;
@@ -763,7 +764,7 @@ static struct bsg_device *bsg_add_device(struct inode
*inode,
bsg_set_block(bd, file);
- atomic_set(&bd->ref_count, 1);
+ refcount_set(&bd->ref_count, 1);
mutex_lock(&bsg_mutex);
hlist_add_head(&bd->dev_list, bsg_dev_idx_hash(iminor(inode)));
@@ -783,7 +784,7 @@ static struct bsg_device *__bsg_get_device(int minor,
struct request_queue *q)
hlist_for_each_entry(bd, bsg_dev_idx_hash(minor), dev_list) {
if (bd->queue == q) {
- atomic_inc(&bd->ref_count);
+ refcount_inc(&bd->ref_count);
goto found;
}
}
--
2.7.4
[PATCH 1/5] block: convert bio.__bi_cnt from atomic_t to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova
Signed-off-by: Hans Liljestrand
Signed-off-by: Kees Cook
Signed-off-by: David Windsor
---
block/bio.c | 6 +++---
fs/btrfs/volumes.c| 2 +-
include/linux/bio.h | 4 ++--
include/linux/blk_types.h | 3 ++-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 888e780..80032f3 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -276,7 +276,7 @@ void bio_init(struct bio *bio, struct bio_vec *table,
{
memset(bio, 0, sizeof(*bio));
atomic_set(&bio->__bi_remaining, 1);
- atomic_set(&bio->__bi_cnt, 1);
+ refcount_set(&bio->__bi_cnt, 1);
bio->bi_io_vec = table;
bio->bi_max_vecs = max_vecs;
@@ -551,12 +551,12 @@ void bio_put(struct bio *bio)
if (!bio_flagged(bio, BIO_REFFED))
bio_free(bio);
else {
- BIO_BUG_ON(!atomic_read(&bio->__bi_cnt));
+ BIO_BUG_ON(!refcount_read(&bio->__bi_cnt));
/*
* last put frees it
*/
- if (atomic_dec_and_test(&bio->__bi_cnt))
+ if (refcount_dec_and_test(&bio->__bi_cnt))
bio_free(bio);
}
}
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 017b67d..cd8a5d5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -446,7 +446,7 @@ static noinline void run_scheduled_bios(struct btrfs_device
*device)
waitqueue_active(&fs_info->async_submit_wait))
wake_up(&fs_info->async_submit_wait);
- BUG_ON(atomic_read(&cur->__bi_cnt) == 0);
+ BUG_ON(refcount_read(&cur->__bi_cnt) == 0);
/*
* if we're doing the sync list, record that our
diff --git a/include/linux/bio.h b/include/linux/bio.h
index d1b04b0..bfd0661 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -229,7 +229,7 @@ static inline void bio_get(struct bio *bio)
{
bio->bi_flags |= (1 << BIO_REFFED);
smp_mb__before_atomic();
- atomic_inc(&bio->__bi_cnt);
+ refcount_inc(&bio->__bi_cnt);
}
static inline void bio_cnt_set(struct bio *bio, unsigned int count)
@@ -238,7 +238,7 @@ static inline void bio_cnt_set(struct bio *bio, unsigned
int count)
bio->bi_flags |= (1 << BIO_REFFED);
smp_mb__before_atomic();
}
- atomic_set(&bio->__bi_cnt, count);
+ refcount_set(&bio->__bi_cnt, count);
}
static inline bool bio_flagged(struct bio *bio, unsigned int bit)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 61339bc..f6e5daa 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -7,6 +7,7 @@
#include
#include
+#include
struct bio_set;
struct bio;
@@ -81,7 +82,7 @@ struct bio {
unsigned short bi_max_vecs;/* max bvl_vecs we can hold */
- atomic_t__bi_cnt; /* pin count */
+ refcount_t __bi_cnt; /* pin count */
struct bio_vec *bi_io_vec; /* the actual vec list */
--
2.7.4
[PATCH 2/5] block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova
Signed-off-by: Hans Liljestrand
Signed-off-by: Kees Cook
Signed-off-by: David Windsor
---
block/blk-tag.c| 8
include/linux/blkdev.h | 3 ++-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/blk-tag.c b/block/blk-tag.c
index 07cc329..d83555e 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL(blk_queue_find_tag);
*/
void blk_free_tags(struct blk_queue_tag *bqt)
{
- if (atomic_dec_and_test(&bqt->refcnt)) {
+ if (refcount_dec_and_test(&bqt->refcnt)) {
BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
bqt->max_depth);
@@ -130,7 +130,7 @@ static struct blk_queue_tag *__blk_queue_init_tags(struct
request_queue *q,
if (init_tag_map(q, tags, depth))
goto fail;
- atomic_set(&tags->refcnt, 1);
+ refcount_set(&tags->refcnt, 1);
tags->alloc_policy = alloc_policy;
tags->next_tag = 0;
return tags;
@@ -180,7 +180,7 @@ int blk_queue_init_tags(struct request_queue *q, int depth,
queue_flag_set(QUEUE_FLAG_QUEUED, q);
return 0;
} else
- atomic_inc(&tags->refcnt);
+ refcount_inc(&tags->refcnt);
/*
* assign it, all done
@@ -225,7 +225,7 @@ int blk_queue_resize_tags(struct request_queue *q, int
new_depth)
* Currently cannot replace a shared tag map with a new
* one, so error out if this is the case
*/
- if (atomic_read(&bqt->refcnt) != 1)
+ if (refcount_read(&bqt->refcnt) != 1)
return -EBUSY;
/*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ddd36b..3efc8cf 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
struct module;
struct scsi_ioctl_command;
@@ -293,7 +294,7 @@ struct blk_queue_tag {
unsigned long *tag_map; /* bit map of free/busy tags */
int max_depth; /* what we will send to device */
int real_max_depth; /* what the array can hold */
- atomic_t refcnt;/* map can be shared */
+ refcount_t refcnt; /* map can be shared */
int alloc_policy; /* tag allocation policy */
int next_tag; /* next tag */
};
--
2.7.4
[PATCH 0/5] v3 block subsystem refcounter conversions
Changes in v3: No changes in patches apart from trivial rebases, but now by default refcount_t = atomic_t and uses all atomic standard operations unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the systems that are critical on performance and cannot accept even slight delay on the refcounter operations. Changes in v2: Not needed WARNs are removed since refcount_t warns by itself. BUG_ONs are left as it is, since refcount_t doesn't bug by default. This series, for block subsystem, replaces atomic_t reference counters with the new refcount_t type and API (see include/linux/refcount.h). By doing this we prevent intentional or accidental underflows or overflows that can lead to use-after-free vulnerabilities. Elena Reshetova (5): block: convert bio.__bi_cnt from atomic_t to refcount_t block: convert blk_queue_tag.refcnt from atomic_t to refcount_t block: convert blkcg_gq.refcnt from atomic_t to refcount_t block: convert io_context.active_ref from atomic_t to refcount_t block: convert bsg_device.ref_count from atomic_t to refcount_t block/bfq-iosched.c| 2 +- block/bio.c| 6 +++--- block/blk-cgroup.c | 2 +- block/blk-ioc.c| 4 ++-- block/blk-tag.c| 8 block/bsg.c| 9 + block/cfq-iosched.c| 4 ++-- fs/btrfs/volumes.c | 2 +- include/linux/bio.h| 4 ++-- include/linux/blk-cgroup.h | 9 - include/linux/blk_types.h | 3 ++- include/linux/blkdev.h | 3 ++- include/linux/iocontext.h | 6 +++--- 13 files changed, 32 insertions(+), 30 deletions(-) -- 2.7.4
Permanently change thin provisioning method from user space?
I'm facing storage system that while generally advertising support for WRITE_SAME_16 with UNMAP in reality fails this request depending on exact volume configuration. This configuration is done on storage side and may happen after host initially discovered LUN. I though about simply overriding kernel auto-detect with UNMAP using sysfs scsi_disk/.../provisioning_mode attribute, but it appears kernel may re-evaluate it in some cases, so changes do not stick. Is it possible to permanently restrict it per-device (or per-vendor/device)? TIA -andrei
Re: [PATCH 1/1] qedi: Add support for offload iSCSI Boot
On 26/06/17, 10:29 PM, "Martin K. Petersen" wrote: > >Nilesh, > >> This patch adds support for offload iSCSI boot (Boot from SAN >> over iSCSI offload). >> >> The dependent qed patches for this support are, >> - qed: Support NVM-image reading API >> - qed: Share additional information with qedf > >You had tagged this for 4.12/scsi-fixes but the above patches are not >present in 4.12. So I presume this would be a candidate for a second >stage 4.13 merge? Yes. > >Patch looks OK to me, but please resubmit with a slightly more verbose >description of the changes and how they work. Done. Resubmitted the patch. Thanks, Nilesh
[PATCH] ses: do not add a device to an enclosure if enclosure_add_links() fails.
The enclosure_add_device() function should fail if it can't
create the relevant sysfs links.
Signed-off-by: Maurizio Lombardi
---
drivers/misc/enclosure.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index d3fe3ea..eb29113 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int
component,
struct device *dev)
{
struct enclosure_component *cdev;
+ int err;
if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,17 @@ int enclosure_add_device(struct enclosure_device *edev,
int component,
if (cdev->dev == dev)
return -EEXIST;
- if (cdev->dev)
+ if (cdev->dev) {
enclosure_remove_links(cdev);
-
- put_device(cdev->dev);
+ put_device(cdev->dev);
+ }
cdev->dev = get_device(dev);
- return enclosure_add_links(cdev);
+ err = enclosure_add_links(cdev);
+ if (err) {
+ put_device(cdev->dev);
+ cdev->dev = NULL;
+ }
+ return err;
}
EXPORT_SYMBOL_GPL(enclosure_add_device);
--
Maurizio Lombardi
[PATCH v1 1/1] qedi: Add support for Boot from SAN over iSCSI offload
This patch adds support for Boot from SAN over iSCSI offload.
The iSCSI boot information in the NVRAM is populated under
/sys/firmware/iscsi_bootX/ using qed NVM-image reading API and
further exported to open-iscsi to perform iSCSI login enabling
boot over offload iSCSI interface in a Boot from SAN environment.
The dependent qed patches for this support are,
- qed: Support NVM-image reading API
- qed: Share additional information with qedf
Signed-off-by: Arun Easi
Signed-off-by: Andrew Vasquez
Signed-off-by: Manish Rangankar
Signed-off-by: Nilesh Javali
---
drivers/scsi/qedi/qedi.h | 17 ++
drivers/scsi/qedi/qedi_main.c | 419 +
drivers/scsi/qedi/qedi_nvm_iscsi_cfg.h | 210 +
3 files changed, 646 insertions(+)
create mode 100644 drivers/scsi/qedi/qedi_nvm_iscsi_cfg.h
diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
index 32632c9..91d2f51 100644
--- a/drivers/scsi/qedi/qedi.h
+++ b/drivers/scsi/qedi/qedi.h
@@ -23,11 +23,17 @@
#include
#include
#include "qedi_version.h"
+#include "qedi_nvm_iscsi_cfg.h"
#define QEDI_MODULE_NAME "qedi"
struct qedi_endpoint;
+#ifndef GET_FIELD2
+#define GET_FIELD2(value, name) \
+ (((value) & (name ## _MASK)) >> (name ## _OFFSET))
+#endif
+
/*
* PCI function probe defines
*/
@@ -66,6 +72,11 @@
#define QEDI_HW_DMA_BOUNDARY 0xfff
#define QEDI_PATH_HANDLE 0xFE000UL
+enum qedi_nvm_tgts {
+ QEDI_NVM_TGT_PRI,
+ QEDI_NVM_TGT_SEC,
+};
+
struct qedi_uio_ctrl {
/* meta data */
u32 uio_hsi_version;
@@ -283,6 +294,8 @@ struct qedi_ctx {
void *bdq_pbl_list;
dma_addr_t bdq_pbl_list_dma;
u8 bdq_pbl_list_num_entries;
+ struct nvm_iscsi_cfg *iscsi_cfg;
+ dma_addr_t nvm_buf_dma;
void __iomem *bdq_primary_prod;
void __iomem *bdq_secondary_prod;
u16 bdq_prod_idx;
@@ -337,6 +350,10 @@ struct qedi_ctx {
bool use_fast_sge;
atomic_t num_offloads;
+#define SYSFS_FLAG_FW_SEL_BOOT 2
+#define IPV6_LEN 41
+#define IPV4_LEN 17
+ struct iscsi_boot_kset *boot_kset;
};
struct qedi_work {
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 09a2946..f07ac1c 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1143,6 +1144,30 @@ static int qedi_setup_int(struct qedi_ctx *qedi)
return rc;
}
+static void qedi_free_nvm_iscsi_cfg(struct qedi_ctx *qedi)
+{
+ if (qedi->iscsi_cfg)
+ dma_free_coherent(&qedi->pdev->dev,
+ sizeof(struct nvm_iscsi_cfg),
+ qedi->iscsi_cfg, qedi->nvm_buf_dma);
+}
+
+static int qedi_alloc_nvm_iscsi_cfg(struct qedi_ctx *qedi)
+{
+ qedi->iscsi_cfg = dma_zalloc_coherent(&qedi->pdev->dev,
+sizeof(struct nvm_iscsi_cfg),
+&qedi->nvm_buf_dma, GFP_KERNEL);
+ if (!qedi->iscsi_cfg) {
+ QEDI_ERR(&qedi->dbg_ctx, "Could not allocate NVM BUF.\n");
+ return -ENOMEM;
+ }
+ QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
+ "NVM BUF addr=0x%p dma=0x%llx.\n", qedi->iscsi_cfg,
+ qedi->nvm_buf_dma);
+
+ return 0;
+}
+
static void qedi_free_bdq(struct qedi_ctx *qedi)
{
int i;
@@ -1183,6 +1208,7 @@ static void qedi_free_global_queues(struct qedi_ctx *qedi)
kfree(gl[i]);
}
qedi_free_bdq(qedi);
+ qedi_free_nvm_iscsi_cfg(qedi);
}
static int qedi_alloc_bdq(struct qedi_ctx *qedi)
@@ -1309,6 +1335,11 @@ static int qedi_alloc_global_queues(struct qedi_ctx
*qedi)
if (rc)
goto mem_alloc_failure;
+ /* Allocate DMA coherent buffers for NVM_ISCSI_CFG */
+ rc = qedi_alloc_nvm_iscsi_cfg(qedi);
+ if (rc)
+ goto mem_alloc_failure;
+
/* Allocate a CQ and an associated PBL for each MSI-X
* vector.
*/
@@ -1673,6 +1704,387 @@ void qedi_reset_host_mtu(struct qedi_ctx *qedi, u16 mtu)
qedi_ops->ll2->start(qedi->cdev, ¶ms);
}
+/**
+ * qedi_get_nvram_block: - Scan through the iSCSI NVRAM block (while accounting
+ * for gaps) for the matching absolute-pf-id of the QEDI device.
+ */
+static struct nvm_iscsi_block *
+qedi_get_nvram_block(struct qedi_ctx *qedi)
+{
+ int i;
+ u8 pf;
+ u32 flags;
+ struct nvm_iscsi_block *block;
+
+ pf = qedi->dev_info.common.abs_pf_id;
+ block = &qedi->iscsi_cfg->block[0];
+ for (i = 0; i < NUM_OF_ISCSI_PF_SUPPORTED; i++, block++) {
+ flags = ((block->id) & NVM_ISCSI_CFG_BLK_CTRL_FLAG_MASK) >>
+ NVM_ISCSI_CFG_BLK_CTRL_FLAG_OFFSET;
+ if (flags & (NVM_ISCSI_CFG_BLK_CTRL_FLAG_IS_NOT_EMPTY |
+
Re: [PATCH v3 0/4] g_NCR5380: PDMA fixes and cleanup
Ondrej,
could this be a partial write (target did not transfer the last byte)?
One would suppose the chip posts a phase mismatch in that case ...
Cheers,
Michael
Am 27.06.2017 um 18:28 schrieb Ondrej Zary:
> On Monday 26 June 2017, Ondrej Zary wrote:
>> On Monday 26 June 2017 09:30:33 Finn Thain wrote:
>>> Ondrej, would you please test this new series?
>>>
>>> Changed since v1:
>>> - PDMA transfer residual is calculated earlier.
>>> - End of DMA flag check is now polled (if there is any residual).
>>>
>>> Changed since v2:
>>> - Bail out of transfer loops when Gated IRQ gets asserted.
>>> - Make udelay conditional on board type.
>>> - Drop sg_tablesize patch due to performance regression.
>>>
>>>
>>> Finn Thain (1):
>>> g_NCR5380: Cleanup comments and whitespace
>>>
>>> Ondrej Zary (3):
>>> g_NCR5380: Fix PDMA transfer size
>>> g_NCR5380: End PDMA transfer correctly on target disconnection
>>> g_NCR5380: Re-work PDMA loops
>>>
>>> drivers/scsi/g_NCR5380.c | 239
>>> +++ 1 file changed, 116
>>> insertions(+), 123 deletions(-)
>
> BTW. I've probably found the DTC write corruption.
> Added the following check (13 is host buffer index register) - and it
> triggers
> sometimes: the value is 1 instead of 0. As we use only 16-bit writes, I don't
> see how the value could ever be odd. Looks like a bug in the chip.
> The index register corrupts during the transfer, not after IRQ or timeout.
> The
> same check at beginning of pwrite() did not trigger.
>
> The index register is not writable so we must(?) reset the PDMA engine to
> recover. However, this quick attempt to fix does not work, maybe we should
> reload the block count and continue?
>
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -595,7 +603,13 @@ static inline int generic_NCR5380_pwrite(struct
> NCR5380_hostdata *hostdata,
> goto out_wait;
> }
> }
> -
> + idx = NCR5380_read(13);
> + if (idx != 0) {
> + printk("host idx=%d, start=%d\n", idx, start);
> + NCR5380_write(hostdata->c400_ctl_status, CSR_RESET);
> + NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
> + goto out_wait;
> + }
> if (hostdata->io_port && hostdata->io_width == 2)
> outsw(hostdata->io_port + hostdata->c400_host_buf,
> src + start, 64);
>
>
