Re: Manual driver binding and unbinding broken for SCSI
On Fri, Feb 17, 2017 at 04:43:56PM -0800, James Bottomley wrote: > This seems to be related to a 0day test we got on the block tree, > details here: > > http://marc.info/?t=14862406881 > > I root caused the above to something not being released when it should > be, so it looks like you have the same problem. It seems to be a > recent commit in the block tree, so could you bisect it since you have > a nice reproducer? These appear to actually be two separate issues. The unbind followed by bind crash only happens with scsi-mq. It reproes since at least 4.0. The unbind followed by a new device coming up crash happens both with and without scsi-mq. The earliest version I was able to check for that was 4.6, which did reproduce. I'll see if I can get some more info on these two issues separately.
Re: [PATCH v5] sd: Check for unaligned partial completion
Christoph, On 2/17/17 17:25, Christoph Hellwig wrote: > Looks fine, > > Reviewed-by: Christoph Hellwig Thanks. Martin, James, It looks like this patch did not make it into final 4.10. This is really bad. Now ZBC support is broken from the start on mpt3sas HBAs (so a huge chunk of the market). Is there any possibility to get this added to 4.10.0 ? Best regards. -- Damien Le Moal, Ph.D. Sr. Manager, System Software Research Group, Western Digital Corporation damien.lem...@wdc.com (+81) 0466-98-3593 (ext. 513593) 1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan www.wdc.com, www.hgst.com
Re: [PATCH V5 net-next 1/2] qed: Add support for hardware offloaded FCoE.
Applied to net-next, thanks.
Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches
On Sun, 19 Feb 2017, Ondrej Zary wrote:
>
> These two patches are enough to make PDMA work with CD-ROM drives on
> 53C400(A), with IRQ enabled or not. They even improve transfer rates
> with HDDs because of bigger block size.
>
Nice! This is a great improvement.
> But DTC436 breaks with CDU-55S, immediately after modprobe.
> 1) Seems that IRQ timing is slightly different so I rewrote the wait for
> the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some
> data to be transferred but
> 2) DTC436 likes to hang (return only 0xff from all registers - like it's
> not even present on the bus) on repeating CSR register reads and maybe some
> other actions too. This problem already appeared before in pwrite() where
> I added the udelay(4) workaround. Now I added udelay(100) to the waits but
> it still crashes sometimes (randomly after tenths or hundreds of MB).
>
That is not a regression, right? If there are no regressions, I'd like to
merge this patch (with some minor tweaks, rebased on the latest driver).
The problem with these Domex chips is that the manufacturer doesn't
respond to repeated requests for information.
If you want to pursue this bug, when working on undocumented proprietary
hardware from manufacturers like this one, what I recommend is to look for
clues in the code for alternative implementations (NetBSD etc) and try
wiggling parameters until the behaviour changes (e.g. reduce the transfer
size to 128 bytes, introduce delays at key places. Probably the same sort
of thing you already tried.) Snooping the device register accessess made
by a DOS/Windows driver is another approach.
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6f9665d..9d0cfd4 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct
> NCR5380_hostdata *hostdata,
> return 0;
> }
>
> +#define G_NCR5380_DMA_MAX_SIZE 32768
Please put this at the top with the other definitions. (As you will
recall, the macro definitions were moved around in the latest driver.)
> 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, G_NCR5380_DMA_MAX_SIZE);
> }
>
> /*
>
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 9d0cfd4..797115e 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -453,15 +453,15 @@ static inline int generic_NCR5380_pread(struct
> NCR5380_hostdata *hostdata,
> int blocks = len / 128;
> int start = 0;
>
> + hostdata->pdma_residual = 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)
> 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;
This algorithm basically says that, if the IRQ is late (or missing) we
will go through the main loop, information transfer and DMA setup steps
again. That's fine, but it means that we have to exit the pread/pwrite
routines with the chip in a suitable state for back-to-back PDMA
transfers.
NCR5380_dma_complete() takes care of things for the 53C80 core, but maybe
the DTC436 logic might need a bit more work (?) For example, if we exit
with a partially filled buffer, will the buffer switching logic break? The
53C400 datasheet mentions a "Resume Transfer Register" which makes me a
bit suspicious about chip state after a DISCONNECT... just a thought.
> while (NCR5380_read(hostdata->c400_ctl_status) &
> CSR_HOST_BUF_NOT_RDY)
> ; /* FIXME - no timeout */
>
> @@ -499,13 +499,13 @@ static inline int generic_NCR5380_pread(struct
> NCR5380_hostdata *hostdata,
>
> if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
>
[PATCH] sd: Fix error handling
Reorder 'out_free' and 'out_free_devt' error handling labels in order to
match the way resources have been allocated.
Fixes: 0dba1314d4f8 ("scsi, block: fix duplicate bdi name registration crashes")
Signed-off-by: Christophe JAILLET
---
drivers/scsi/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..99e12061a6fb 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3213,10 +3213,10 @@ static int sd_probe(struct device *dev)
sd_devt = NULL;
out_put:
put_disk(gd);
- out_free:
- kfree(sdkp);
out_free_devt:
kfree(sd_devt);
+ out_free:
+ kfree(sdkp);
out:
scsi_autopm_put_device(sdp);
return error;
--
2.9.3
Re: scsi: BUG in scsi_init_io
On Sat, Feb 18, 2017 at 11:15 PM, Al Viro wrote: > > AFAICS, the minimal fix would be something like this: Applied, along with getting rid of the BUG_ON() that made this bug much worse than it would have been without it. Linus
Re: scsi: BUG in scsi_init_io
On Tue, Jan 31, 2017 at 7:41 AM, James Bottomley wrote: > > It is a kernel bug and it should not be user triggerable, so it should > have a warn_on or bug_on. Hell NO. Christ, James, listen to yourself. What you are basically saying when you say it should be a BUG_ON() is "This shouldn't happen, but if it ever does happen, let's just turn our mistaken assumptions into a dead machine that is really hard to debug". Because a BUG_ON() effectively kills the machine if the call chain has some locks held. In the SCSI layer, that generally means that there will be no logged oops either, because any locks held likely just killed your filesystem or disk subsystem, so now that oops is basically not even likely to be reported by most normal users. So stop this "should have a bug_on". In fact, since this apparently _is_ easily user-triggerable, it damn well shouldn't have a warn_on either. At most, a WARN_ON_ONCE(), so that we might get reports of _what_ the bad call chain is, but we will never kill the machine and we will *not* give people the ability to randomly spam the system logs. BUG_ON() needs to die. People need to realize that it is a _problem_, and that it makes any bugs _worse_. Don't do it. The only valid reason for BUG_ON() is when some very core data structure is _so_ corrupt that you can't even continue, because you simply can't even return an error and there's no way for you to just say "log it once and continue". And by that I don't mean some random value you have in a request. I mean literally "this is a really core data structure, and I simply _cannot_ continue" (where that "cannot" is about an actual physical impossibility, not a "I could continue but I think this is serious"). Anything else is a "return error, possibly with a WARN_ON() to let people know that bad things are going on". Basically, BUG_ON() should be in core kernel code. Not in drivers. And even in core kernel code, it's likely wrong. Linus
Re: scsi: BUG in scsi_init_io
On Sun, Feb 19, 2017 at 07:15:27AM +, Al Viro wrote: > The root cause is unfixable without access to TARDIS and dose of > antipsychotics sufficient to prevent /dev/sg API creation. > > What happens is that write to /dev/sg is given a request with non-zero > ->iovec_count combined with zero ->dxfer_len. Or with ->dxferp pointing > to an array full of empty iovecs. > > AFAICS, the minimal fix would be something like this: > > YAMissingSanityCheck in /dev/sg > > write permission to /dev/sg shouldn't be equivalent to the ability to trigger > BUG_ON() while holding spinlocks... Looks fine to me: Reviewed-by: Christoph Hellwig The other thing we really need to consider is to finally merge the SG_IO implementations for /dev/sg with the common block layer one.
[Bug 60644] MPT2SAS drops all HDDs when under high I/O
https://bugzilla.kernel.org/show_bug.cgi?id=60644 --- Comment #52 from o...@ojab.ru --- https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ffdadd68af5a397b8a52289ab39d62e1acb39e63 Patch is merged, bug can be closed. -- You are receiving this mail because: You are watching the assignee of the bug.
Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches
On Sunday 19 February 2017 00:27:55 Finn Thain wrote: > On Sat, 18 Feb 2017, Ondrej Zary wrote: > > On Friday 17 February 2017 23:38:12 Finn Thain wrote: > > > On Thu, 16 Feb 2017, Ondrej Zary wrote: > > > > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote: > > > > [...] > > > > > > > > > Are you trying to figure out which commands are going to > > > > > disconnect during a transfer? This is really a function of the > > > > > firmware in the target; there are no good heuristics AFAICT, so > > > > > the PDMA algorithm has to be robust. mac_scsi has to cope with > > > > > this too. > > > > > > > > > > Does the problem go away when you assign no IRQ? When > > > > > instance->irq == NO_IRQ, the core driver will inhibit disconnect > > > > > privileges. > > > > > > > > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ > > > > enabled, "dd if=/dev/sr0 of=anything" stops after a while. > > > > > > When you say "stops", do you mean an infinite loop in > > > generic_NCR5380_pread or does the loop complete (which would > > > presumably leave the target stuck in DATA IN phase, and a scsi command > > > timeout would probably follow after 30 seconds...) > > > > I've added timeouts to the possibly-infinite loops. It times out waiting > > for the host buffer to become ready. > > In mac_scsi you'll find that the PDMA loop exploits the 15ms poll_politely > time limit to give the target device time to catch up. You might want to > do something similar. > > > Then everything breaks. Now I found why: pread() returns without waiting > > for the 53C80 registers to be ready. > > Ouch! You can't return control to the core driver when the 53C80 core is > unavailable. That would need special handling: the core driver would have > to fail the scsi command and reset the device (and bus), based on the > result you return from NCR5380_dma_recv_setup/NCR5380_dma_send_setup. > > > Adding the wait allows to continue in PIO mode with "tag#0 switching to > > slow handshake". > > I don't think this is the code path you want. The target isn't really > broken. But yes, we could use PIO as a slow workaround for fragile PDMA > logic. Yes, we don't want that. > > > > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c. > > > > > > You can use NCR5380_print() to get a decoded register dump. > > > > > > When I decode the above values I get, > > > > > > BASR = 0x10 = BASR_IRQ > > > MODE = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE > > > STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO > > > > > > Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a > > > phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN, > > > which shows that the target has given up on the DATA IN phase and is > > > presumably trying to send a DISCONNECT message. > > > > Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors. > > When the 2nd sector is not ready in the drive internal buffer, the drive > > probably disconnects which breaks the fragile pdma transfer. When the > > transfersize is limited to 2048 bytes, the problem goes away. > > OK. > > > The problem also went away with enabled interrupts because I had some > > debug printks added which were slowing down the transfer enough for the > > drive (SONY CDU-55S) to keep up and not disconnect. Got the same result > > by inserting udelay(100) after each 128 byte block trasnferred in > > pread(). > > Well, yeah, if you change the timing and omit to mention that, as well as > changing the spinlock_irq_save/restore pairs, then it's going to be > difficult for me to make sense of your results. Anyway, I'm glad that you > got to the bottom of this mystery. > > > > > When I enable interrupts during PDMA (like the removed UNSAFE macro > > > > did), the problems go away. I see an IRQ after each pread call. These two patches are enough to make PDMA work with CD-ROM drives on 53C400(A), with IRQ enabled or not. They even improve transfer rates with HDDs because of bigger block size. But DTC436 breaks with CDU-55S, immediately after modprobe. 1) Seems that IRQ timing is slightly different so I rewrote the wait for the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some data to be transferred but 2) DTC436 likes to hang (return only 0xff from all registers - like it's not even present on the bus) on repeating CSR register reads and maybe some other actions too. This problem already appeared before in pwrite() where I added the udelay(4) workaround. Now I added udelay(100) to the waits but it still crashes sometimes (randomly after tenths or hundreds of MB). diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c index 6f9665d..9d0cfd4 100644 --- a/drivers/scsi/g_NCR5380.c +++ b/drivers/scsi/g_NCR5380.c @@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata, return 0; } +#define G_NCR5380_DMA_MAX_SIZE 32768 static int generic_NCR5380_dma_xfer_len(struct
