Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM
11.09.2014 03:10, Luis R. Rodriguez wrote: Tom, thanks for reviewing this! My reply below! On Tue, Sep 9, 2014 at 11:46 PM, Tom Gundersen wrote: On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez wrote: On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley wrote: On Tue, 2014-09-09 at 12:16 -0700, Luis R. Rodriguez wrote: On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley wrote: If we want to sort out some sync/async mechanism for probing devices, as an agreement between the init systems and the kernel, that's fine, but its a to-be negotiated enhancement. Unfortunately as Tejun notes the train has left which already made assumptions on this. Well, that's why it's a bug. It's a material regression impacting users. Indeed. I believe the issue with this regression however was that the original commit e64fae55 (January 2012) was only accepted by *kernel folks* to be a real regression until recently. Just for the record, this only caused user-visible problems after kernel commit 786235ee (November 2013), right? Another one was cxgb4: https://bugzilla.novell.com/show_bug.cgi?id=877622 SLE12 does not yet have commit 786235ee merged. Benjamim did some hard work to debug this and trace the kill down to systemd-udev. A debug kernel build has been provided now to try to pick up exactly on the place where the kill was received, but its at least clear this came from systemd. More than two years have gone by on growing design and assumptions on top of that original commit. I'm not sure if *systemd folks* yet believe its was a design regression? I don't think so. udev should not allow its workers to run for an unbounded length of time. Whether the upper bound should be 30, 60, 180 seconds or something else is up for debate (currently it is 60, but if that is too short for some drivers we could certainly revisit that). That's the thing -- the timeout was put in place under the assumption probe was asyncronous and its not, the driver core issues both module init *and* probe together, the loader has to wait. That alone makes the timeout a design flaw, and then systemd carried on top of that design over two years after that. Its not systemd's fault, its just that we never spoke about this as a design thing broadly and we should have, and I will mention that even when the first issues creeped up, the issue was still tossed back a driver problems. It was only until recently that we realized that both init and probe run together that we've been thinking about this problem differently. Systemd was trying to ensure init on driver don't take long but its not init that is taking long, its probe, and probe gets then penalized as the driver core batches both init and probe synchronously before finishing module loading. Furthermore as clarified by Tejun random userland is known to exist that will wait indefinitely for module loading under the simple assumption things *are done synchronously*, and its precisely why we can't just blindly enable async probe upstream through a new driver boolean as it can be unfair to this old userland. What is being evaluated is to enable aync probe for *all* drivers through a new general system-wide option. We cannot regress old userspace and assumptions but we can create a new shiny universe. Moreover, it seems from this discussion that the aim is (still) that insmod should be near-instantaneous (i.e., not wait for probe), The only reason that is being discussed is that systemd has not accepted the timeout as a system design despite me pointing out the original design flaw recently and at this point even if was accepted as a design flaw it seems its too late. The approach taken to help make all drivers async is simply an afterthought to give systemd what it *thought* was in place, and it by no measure should be considered the proper fix to the regression introduced by systemd, it may perhaps be the right step long term for systemd systems given it goes with what it assumed was there, but the timeout was flawed. Its not clear if systemd can help with old kernels, it seems the ship has sailed and there seems no options but for folks to work around that -- unless of course some reasonable solution is found which doesn't break current systemd design? so it seems to me that the basic design is correct and all we need is some temporary work-around and a way to better detect misbehaving drivers? As part of this series I addressed hunting for the "misbehaving drivers" in-kernel as I saw no progress on the systemd side of things to non-fatally detect "misbehaving drivers" despite my original RFCs and request for advice. I quote "misbehaving drivers" as its a flawed view to consider them misbehaving now in light of clarifications of how the driver core works in that it batches both init and probe together always and we can't be penalizing long probes due to the fact long probes are simply fine. My patch to pick up "misbehaving drivers" drivers on the kernel front by picking up s
Re: lk 3.17-rc4 blk_mq large write problems
On 14-09-10 11:48 PM, Elliott, Robert (Server Storage) wrote:
-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Jens Axboe
Sent: Wednesday, September 10, 2014 9:00 PM
To: dgilb...@interlog.com; Christoph Hellwig
Cc: SCSI development list
Subject: Re: lk 3.17-rc4 blk_mq large write problems
On 2014-09-10 18:58, Douglas Gilbert wrote:
On 14-09-10 11:41 AM, Christoph Hellwig wrote:
While it might not help with a blown stack, can you give the patch
below
a try? I tries to solve a problem where the timeout handler hits
before we've fully set up a command. While I'd like to understand
the
root cause of why we're hitting it as well, I'd also really to fix
that
race. It would also be good to get a gdb listing of the exact area in
scsi_times_out listed in the oops.
RIP: 0010:[] []
scsi_times_out+0xe/0x2e0
(gdb) disassemble scsi_times_out
Dump of assembler code for function scsi_times_out:
0x8127d030 <+0>:push %rbp
0x8127d031 <+1>:mov$0x2007,%esi
0x8127d036 <+6>:push %rbx
0x8127d037 <+7>:mov0xf8(%rdi),%rbx
0x8127d03e <+14>:mov(%rbx),%rax
0x8127d041 <+17>:mov%rbx,%rdi
0x8127d044 <+20>:mov(%rax),%rbp
0x8127d047 <+23>:callq 0x81277c70
0x8127d04c <+28>:cmpl $0x,0x154(%rbp)
0x8127d053 <+35>:je 0x8127d05f
...
which seems to agree 'objdump -drS scsi_error.o':
28b0 :
28b0:55 push %rbp
28b1:be 07 20 00 00 mov$0x2007,%esi
28b6:53 push %rbx
28b7:48 8b 9f f8 00 00 00 mov0xf8(%rdi),%rbx
28be:48 8b 03 mov(%rbx),%rax
28c1:48 89 df mov%rbx,%rdi
28c4:48 8b 28 mov(%rax),%rbp
28c7:e8 00 00 00 00 callq 28cc
28c8: R_X86_64_PC32scsi_log_completion-0x4
28cc:83 bd 54 01 00 00 ff cmpl $0x,0x154(%rbp)
This would be more useful if you had DEBUGINFO enabled. At least it
would save use some time :-)
On my system, that function compiles to:
enum blk_eh_timer_return scsi_times_out(struct request *req)
{
2580: 55 push %rbp
2581: 48 89 e5mov%rsp,%rbp
2584: 41 55 push %r13
2586: 41 54 push %r12
2588: 53 push %rbx
2589: 48 83 ec 08 sub$0x8,%rsp
258d: e8 00 00 00 00 callq 2592
258e: R_X86_64_PC32 mcount-0x4
struct scsi_cmnd *scmd = req->special;
2592: 48 8b 9f f8 00 00 00mov0xf8(%rdi),%rbx
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
struct Scsi_Host *host = scmd->device->host;
2599: 48 8b 03mov(%rbx),%rax
259c: 4c 8b 20mov(%rax),%r12
259f: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
So, Doug's +0xe corresponds to +0x19 here.
258d-2591 and the extra code preceding them are for stack
overflow checking, which I have enabled, and the location
for the optional ftrace jump if tracing this function is
turned on.
2592 is scmd = req->special. %rdi is req, the first and
only function argument (per the x86_64 abi). 0xf8(%rdi)
is referencing req->special. req must be OK, since that's
not the instruction pointer address that failed.
2599 is dereferencing req->special (i.e., scmd) to set host.
req->special must contain NULL, causing the exception
at this IP.
As a short-term workaround, it might be better to just let
this function exit if scmd is NULL, assuming that it was
already dealt with properly. That would still be misled
if the race condition, whatever it may be, allows scmd to
be reused before this function runs; this function would
then operate on a future command.
Switching back to a SATA SSD gives me the same
"scsi_times_out+0xe" NULL de-reference oops shown at the
start of this thread.
(gdb) list *(scsi_times_out+0xe)
0x812804fe is in scsi_times_out (drivers/scsi/scsi_error.c:277).
272 */
273 enum blk_eh_timer_return scsi_times_out(struct request *req)
274 {
275 struct scsi_cmnd *scmd = req->special;
276 enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
277 struct Scsi_Host *host = scmd->device->host;
278
279 trace_scsi_dispatch_cmd_timeout(scmd);
280 scsi_log_completion(scmd, TIMEOUT_ERROR);
As Rob suggested.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.htm
Re: [PATCH v3 16/17] arcmsr: support new adapter ARC12x4 series
On Wed, 2014-09-10 at 11:58 +0200, Tomas Henzl wrote:
> On 09/09/2014 06:30 PM, Christoph Hellwig wrote:
> > Ching,
> >
> > do you have a chance to address Thomas second concern below? As
> > far as I can tell (Thomas, please correct me) that's the last
> > outstanding concern, and I'd really like to merge the arcmsr updates
> > for the Linux 3.18 merge window.
>
> Correct, still awaiting a response.
Christoph, Tomas,
Sorry for the late reply.
I think I misunderstand Tomas' meaning.
The spin lock in arcmsr_hbaD_polling_ccbdone() is necessary to protect
doneq_index and have to be modified as following.
static int arcmsr_hbaD_polling_ccbdone(struct AdapterControlBlock *acb,
struct CommandControlBlock *poll_ccb)
{
bool error;
uint32_t poll_ccb_done = 0, poll_count = 0, flag_ccb, ccb_cdb_phy;
int rtn, doneq_index, index_stripped, outbound_write_pointer, toggle;
unsigned long flags;
struct ARCMSR_CDB *arcmsr_cdb;
struct CommandControlBlock *pCCB;
struct MessageUnit_D *pmu = acb->pmuD;
polling_hbaD_ccb_retry:
poll_count++;
while (1) {
spin_lock_irqsave(&acb->doneq_lock, flags);
outbound_write_pointer = pmu->done_qbuffer[0].addressLow + 1;
doneq_index = pmu->doneq_index;
if ((outbound_write_pointer & 0xFFF) == (doneq_index & 0xFFF)) {
spin_unlock_irqrestore(&acb->doneq_lock, flags);
if (poll_ccb_done) {
rtn = SUCCESS;
break;
} else {
msleep(25);
if (poll_count > 40) {
rtn = FAILED;
break;
}
goto polling_hbaD_ccb_retry;
}
}
toggle = doneq_index & 0x4000;
index_stripped = (doneq_index & 0xFFF) + 1;
index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE;
pmu->doneq_index = index_stripped ? (index_stripped | toggle) :
((index_stripped + 1) | (toggle ^ 0x4000));
spin_unlock_irqrestore(&acb->doneq_lock, flags);
doneq_index = pmu->doneq_index;
flag_ccb = pmu->done_qbuffer[doneq_index & 0xFFF].addressLow;
ccb_cdb_phy = (flag_ccb & 0xFFF0);
arcmsr_cdb = (struct ARCMSR_CDB *)(acb->vir2phy_offset +
ccb_cdb_phy);
pCCB = container_of(arcmsr_cdb, struct CommandControlBlock,
arcmsr_cdb);
poll_ccb_done |= (pCCB == poll_ccb) ? 1 : 0;
if ((pCCB->acb != acb) ||
(pCCB->startdone != ARCMSR_CCB_START)) {
if (pCCB->startdone == ARCMSR_CCB_ABORTED) {
pr_notice("arcmsr%d: scsi id = %d "
"lun = %d ccb = '0x%p' poll command "
"abort successfully\n"
, acb->host->host_no
, pCCB->pcmd->device->id
, (u32)pCCB->pcmd->device->lun
, pCCB);
pCCB->pcmd->result = DID_ABORT << 16;
arcmsr_ccb_complete(pCCB);
continue;
}
pr_notice("arcmsr%d: polling an illegal "
"ccb command done ccb = '0x%p' "
"ccboutstandingcount = %d\n"
, acb->host->host_no
, pCCB
, atomic_read(&acb->ccboutstandingcount));
continue;
}
error = (flag_ccb & ARCMSR_CCBREPLY_FLAG_ERROR_MODE1)
? true : false;
arcmsr_report_ccb_state(acb, pCCB, error);
}
return rtn;
}
>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: lk 3.17-rc4 blk_mq large write problems
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Jens Axboe
> Sent: Wednesday, September 10, 2014 9:00 PM
> To: dgilb...@interlog.com; Christoph Hellwig
> Cc: SCSI development list
> Subject: Re: lk 3.17-rc4 blk_mq large write problems
>
> On 2014-09-10 18:58, Douglas Gilbert wrote:
> > On 14-09-10 11:41 AM, Christoph Hellwig wrote:
> >> While it might not help with a blown stack, can you give the patch
> below
> >> a try? I tries to solve a problem where the timeout handler hits
> >> before we've fully set up a command. While I'd like to understand
> the
> >> root cause of why we're hitting it as well, I'd also really to fix
> that
> >> race. It would also be good to get a gdb listing of the exact area in
> >> scsi_times_out listed in the oops.
> >
> > RIP: 0010:[] []
> > scsi_times_out+0xe/0x2e0
> >
> > (gdb) disassemble scsi_times_out
> > Dump of assembler code for function scsi_times_out:
> > 0x8127d030 <+0>:push %rbp
> > 0x8127d031 <+1>:mov$0x2007,%esi
> > 0x8127d036 <+6>:push %rbx
> > 0x8127d037 <+7>:mov0xf8(%rdi),%rbx
> > 0x8127d03e <+14>:mov(%rbx),%rax
> > 0x8127d041 <+17>:mov%rbx,%rdi
> > 0x8127d044 <+20>:mov(%rax),%rbp
> > 0x8127d047 <+23>:callq 0x81277c70
> >
> > 0x8127d04c <+28>:cmpl $0x,0x154(%rbp)
> > 0x8127d053 <+35>:je 0x8127d05f
> >
> > ...
> >
> > which seems to agree 'objdump -drS scsi_error.o':
> >
> > 28b0 :
> > 28b0:55 push %rbp
> > 28b1:be 07 20 00 00 mov$0x2007,%esi
> > 28b6:53 push %rbx
> > 28b7:48 8b 9f f8 00 00 00 mov0xf8(%rdi),%rbx
> > 28be:48 8b 03 mov(%rbx),%rax
> > 28c1:48 89 df mov%rbx,%rdi
> > 28c4:48 8b 28 mov(%rax),%rbp
> > 28c7:e8 00 00 00 00 callq 28cc
>
> > 28c8: R_X86_64_PC32scsi_log_completion-0x4
> > 28cc:83 bd 54 01 00 00 ff cmpl $0x,0x154(%rbp)
>
> This would be more useful if you had DEBUGINFO enabled. At least it
> would save use some time :-)
>
On my system, that function compiles to:
enum blk_eh_timer_return scsi_times_out(struct request *req)
{
2580: 55 push %rbp
2581: 48 89 e5mov%rsp,%rbp
2584: 41 55 push %r13
2586: 41 54 push %r12
2588: 53 push %rbx
2589: 48 83 ec 08 sub$0x8,%rsp
258d: e8 00 00 00 00 callq 2592
258e: R_X86_64_PC32 mcount-0x4
struct scsi_cmnd *scmd = req->special;
2592: 48 8b 9f f8 00 00 00mov0xf8(%rdi),%rbx
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
struct Scsi_Host *host = scmd->device->host;
2599: 48 8b 03mov(%rbx),%rax
259c: 4c 8b 20mov(%rax),%r12
259f: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
So, Doug's +0xe corresponds to +0x19 here.
258d-2591 and the extra code preceding them are for stack
overflow checking, which I have enabled, and the location
for the optional ftrace jump if tracing this function is
turned on.
2592 is scmd = req->special. %rdi is req, the first and
only function argument (per the x86_64 abi). 0xf8(%rdi)
is referencing req->special. req must be OK, since that's
not the instruction pointer address that failed.
2599 is dereferencing req->special (i.e., scmd) to set host.
req->special must contain NULL, causing the exception
at this IP.
As a short-term workaround, it might be better to just let
this function exit if scmd is NULL, assuming that it was
already dealt with properly. That would still be misled
if the race condition, whatever it may be, allows scmd to
be reused before this function runs; this function would
then operate on a future command.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
-Original Message- From: Hans de Goede [mailto:hdego...@redhat.com] Sent: Wednesday, September 10, 2014 8:33 PM To: Peter Hurley; Greg Kroah-Hartman Cc: linux-...@vger.kernel.org; linux-scsi@vger.kernel.org; sta...@vger.kernel.org; Sharma, Sanjeev; Peter Zijlstra; Ingo Molnar Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held() Hi, On 09/10/2014 04:38 PM, Peter Hurley wrote: > [ +cc Peter Zijlstra, Ingo Molnar ] > > On 09/10/2014 07:46 AM, Hans de Goede wrote: >> From: Sanjeev Sharma >> >> On some architecture spin_is_locked() always return false in >> uniprocessor configuration and therefore it would be advise to >> replace with lockdep_assert_held(). >> >> Signed-off-by: Sanjeev Sharma >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/storage/uas.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 3f42785..05b2d8e 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info >> *devinfo, >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp); >> >> uas_log_cmd_state(cmnd, caller); >> -WARN_ON_ONCE(!spin_is_locked(&devinfo->lock)); >> +lockdep_assert_held(&devinfo->lock); > > This change isn't equivalent. > > lockdep_assert_held() will continue to emit warnings; ie., there is no > "once" functionality. Same for the other changes below. >Given that these should really never ever happen, that is not really a problem >IMHO. The idea ws to replace the wrong use of spin_is_locked with some other >sanity check, preferably a light weight one, the once behavior is not that >important. I am totally agree with the hans and that was my intention for UP system. Regards Sanjeev Sharma
RE: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
-Original Message- From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] Sent: Wednesday, September 10, 2014 7:21 PM To: Hans de Goede Cc: Oliver Neukum; linux-...@vger.kernel.org; linux-scsi@vger.kernel.org; sta...@vger.kernel.org; Sharma, Sanjeev Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held() On Wed, Sep 10, 2014 at 03:15:41PM +0200, Hans de Goede wrote: > Hi, > > On 09/10/2014 02:54 PM, Oliver Neukum wrote: > > On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 09/10/2014 01:56 PM, Oliver Neukum wrote: > >>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: > Hi, > > Note this series is NOT intended for stable, but I accidentally > had "cc = sta...@vger.kernel.org" in my .git/config when sending > this series, please ignore for stable. > > NACK for stable. > >>> > >>> If this is not for stable, what do you intend to do about the > >>> problems in stable? For example patch#01 of this series looks like > >>> clear stable material to me. > >> > >> The plan for stable is mostly, as lame as that is, to make sure we > >> get all the right quirks in place so that error handling does not > >> get triggered, for now. > > > > How? A medium can be defect. Short of entirely disabling it, error > > handling will be triggered. > > I agree that this is a concern, but defective disks are not the norm. > All the bugs I've received sofar seem to be about incompatibilities > between the Linux uas/scsi stack and the device, not defective mediums. > > >> I agree that once this set has seen wider testing, we should > >> reconsider, and probably add it, to stable. But at this point in > >> time I'm worried that it may cause regressions, and as such it is > >> not stable material atm IHMO. > > > > Well, we would exchange something known to work imperfectly for > > something feared to work imperfectly. > > True. Note as said I'm not against this going into stable, I just > don't want to rush it into stable. So first lets get it reviewed and > into > 3.18 (and see how it works for the users who have been having troubles > sofar, see my request for testing), and then see from there. > > I assume that you agree that this is (way) too late for 3.17? Yes it is. And I agree, let's test this out first, and if it solves problems, _then_ we can backport it to stable as needed. thanks for the patches, I'll queue them up for 3.18. Thanks all There are some more areas in Kernel which need replacement and I will look into those area too. Regards Sanjeev Sharma -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] megaraid_sas : MFI MPT linked list corruption fix
On Wed, Sep 10, 2014 at 8:36 PM, Tomas Henzl wrote:
> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>> Problem statement:
>> MFI link list in megaraid_sas driver is used from mfi-mpt pass-through
>> commands.
>> This list can be corrupted due to many possible race conditions in driver and
>> eventually we may see kernel panic.
>>
>> One example -
>> MFI frame is freed from calling process as driver send command via polling
>> method and interrupt
>> for that command comes after driver free mfi frame (actually even after some
>> other context reuse
>> the mfi frame). When driver receive MPT frame in ISR, driver will be using
>> the index of MFI and
>> access that MFI frame and finally in-used MFI frame’s list will be corrupted.
>>
>> High level description of new solution -
>> Free MFI and MPT command from same context.
>> Free both the command either from process (from where mfi-mpt pass-through
>> was called) or from
>> ISR context. Do not split freeing of MFI and MPT, because it creates the
>> race condition which
>> will do MFI/MPT list corruption.
>>
>> Renamed the cmd_pool_lock which is used in instance as well as fusion with
>> below name.
>> mfi_pool_lock and mpt_pool_lock to add more code readability.
>>
>> Signed-off-by: Sumit Saxena
>> Signed-off-by: Kashyap Desai
>> ---
>> drivers/scsi/megaraid/megaraid_sas.h| 25 +++-
>> drivers/scsi/megaraid/megaraid_sas_base.c | 196
>>
>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 95 ++
>> drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 +-
>> 4 files changed, 235 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>> b/drivers/scsi/megaraid/megaraid_sas.h
>> index 156d4b9..f99db18 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas.h
>> +++ b/drivers/scsi/megaraid/megaraid_sas.h
>> @@ -1016,6 +1016,12 @@ struct megasas_ctrl_info {
>>
>> #define VD_EXT_DEBUG 0
>>
>> +enum MR_MFI_MPT_PTHR_FLAGS {
>> + MFI_MPT_DETACHED = 0,
>> + MFI_LIST_ADDED = 1,
>> + MFI_MPT_ATTACHED = 2,
>> +};
>> +
>> /* Frame Type */
>> #define IO_FRAME 0
>> #define PTHRU_FRAME 1
>> @@ -1033,7 +1039,7 @@ struct megasas_ctrl_info {
>> #define MEGASAS_IOCTL_CMD0
>> #define MEGASAS_DEFAULT_CMD_TIMEOUT 90
>> #define MEGASAS_THROTTLE_QUEUE_DEPTH 16
>> -
>> +#define MEGASAS_BLOCKED_CMD_TIMEOUT 60
>> /*
>> * FW reports the maximum of number of commands that it can accept (maximum
>> * commands that can be outstanding) at any time. The driver must report a
>> @@ -1652,7 +1658,7 @@ struct megasas_instance {
>> struct megasas_cmd **cmd_list;
>> struct list_head cmd_pool;
>> /* used to sync fire the cmd to fw */
>> - spinlock_t cmd_pool_lock;
>> + spinlock_t mfi_pool_lock;
>> /* used to sync fire the cmd to fw */
>> spinlock_t hba_lock;
>> /* used to synch producer, consumer ptrs in dpc */
>> @@ -1839,6 +1845,11 @@ struct megasas_cmd {
>>
>> struct list_head list;
>> struct scsi_cmnd *scmd;
>> +
>> + void *mpt_pthr_cmd_blocked;
>> + atomic_t mfi_mpt_pthr;
>> + u8 is_wait_event;
>> +
>> struct megasas_instance *instance;
>> union {
>> struct {
>> @@ -1927,4 +1938,14 @@ int megasas_set_crash_dump_params(struct
>> megasas_instance *instance,
>> void megasas_free_host_crash_buffer(struct megasas_instance *instance);
>> void megasas_fusion_crash_dump_wq(struct work_struct *work);
>>
>> +void megasas_return_cmd_fusion(struct megasas_instance *instance,
>> + struct megasas_cmd_fusion *cmd);
>> +int megasas_issue_blocked_cmd(struct megasas_instance *instance,
>> + struct megasas_cmd *cmd, int timeout);
>> +void __megasas_return_cmd(struct megasas_instance *instance,
>> + struct megasas_cmd *cmd);
>> +
>> +void megasas_return_mfi_mpt_pthr(struct megasas_instance *instance,
>> + struct megasas_cmd *cmd_mfi, struct megasas_cmd_fusion *cmd_fusion);
>> +
>> #endif /*LSI_MEGARAID_SAS_H */
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>> b/drivers/scsi/megaraid/megaraid_sas_base.c
>> index 086beee..50d69eb 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>> @@ -210,43 +210,66 @@ struct megasas_cmd *megasas_get_cmd(struct
>> megasas_instance
>> unsigned long flags;
>> struct megasas_cmd *cmd = NULL;
>>
>> - spin_lock_irqsave(&instance->cmd_pool_lock, flags);
>> + spin_lock_irqsave(&instance->mfi_pool_lock, flags);
>>
>> if (!list_empty(&instance->cmd_pool)) {
>> cmd = list_entry((&instance->cmd_pool)->next,
>>struct megasas_cmd, list);
>> list_del_init(&cmd->list);
>> + atomic_set(&cmd->mfi_mpt_pthr, MFI_MPT_DETACHED);
>> } else {
>> printk(KERN_ER
Re: lk 3.17-rc4 blk_mq large write problems
On 2014-09-10 18:58, Douglas Gilbert wrote: On 14-09-10 11:41 AM, Christoph Hellwig wrote: While it might not help with a blown stack, can you give the patch below a try? I tries to solve a problem where the timeout handler hits before we've fully set up a command. While I'd like to understand the root cause of why we're hitting it as well, I'd also really to fix that race. It would also be good to get a gdb listing of the exact area in scsi_times_out listed in the oops. RIP: 0010:[] [] scsi_times_out+0xe/0x2e0 (gdb) disassemble scsi_times_out Dump of assembler code for function scsi_times_out: 0x8127d030 <+0>:push %rbp 0x8127d031 <+1>:mov$0x2007,%esi 0x8127d036 <+6>:push %rbx 0x8127d037 <+7>:mov0xf8(%rdi),%rbx 0x8127d03e <+14>:mov(%rbx),%rax 0x8127d041 <+17>:mov%rbx,%rdi 0x8127d044 <+20>:mov(%rax),%rbp 0x8127d047 <+23>:callq 0x81277c70 0x8127d04c <+28>:cmpl $0x,0x154(%rbp) 0x8127d053 <+35>:je 0x8127d05f ... which seems to agree 'objdump -drS scsi_error.o': 28b0 : 28b0:55 push %rbp 28b1:be 07 20 00 00 mov$0x2007,%esi 28b6:53 push %rbx 28b7:48 8b 9f f8 00 00 00 mov0xf8(%rdi),%rbx 28be:48 8b 03 mov(%rbx),%rax 28c1:48 89 df mov%rbx,%rdi 28c4:48 8b 28 mov(%rax),%rbp 28c7:e8 00 00 00 00 callq 28cc 28c8: R_X86_64_PC32scsi_log_completion-0x4 28cc:83 bd 54 01 00 00 ff cmpl $0x,0x154(%rbp) This would be more useful if you had DEBUGINFO enabled. At least it would save use some time :-) -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On 14-09-10 11:41 AM, Christoph Hellwig wrote: While it might not help with a blown stack, can you give the patch below a try? I tries to solve a problem where the timeout handler hits before we've fully set up a command. While I'd like to understand the root cause of why we're hitting it as well, I'd also really to fix that race. It would also be good to get a gdb listing of the exact area in scsi_times_out listed in the oops. RIP: 0010:[] [] scsi_times_out+0xe/0x2e0 (gdb) disassemble scsi_times_out Dump of assembler code for function scsi_times_out: 0x8127d030 <+0>: push %rbp 0x8127d031 <+1>: mov$0x2007,%esi 0x8127d036 <+6>: push %rbx 0x8127d037 <+7>: mov0xf8(%rdi),%rbx 0x8127d03e <+14>: mov(%rbx),%rax 0x8127d041 <+17>: mov%rbx,%rdi 0x8127d044 <+20>: mov(%rax),%rbp 0x8127d047 <+23>: callq 0x81277c70 0x8127d04c <+28>: cmpl $0x,0x154(%rbp) 0x8127d053 <+35>: je 0x8127d05f ... which seems to agree 'objdump -drS scsi_error.o': 28b0 : 28b0: 55 push %rbp 28b1: be 07 20 00 00 mov$0x2007,%esi 28b6: 53 push %rbx 28b7: 48 8b 9f f8 00 00 00mov0xf8(%rdi),%rbx 28be: 48 8b 03mov(%rbx),%rax 28c1: 48 89 dfmov%rbx,%rdi 28c4: 48 8b 28mov(%rax),%rbp 28c7: e8 00 00 00 00 callq 28cc 28c8: R_X86_64_PC32 scsi_log_completion-0x4 28cc: 83 bd 54 01 00 00 ffcmpl $0x,0x154(%rbp) From: Christoph Hellwig Subject: blk-mq: call blk_mq_start_request from ->queue_rq When we call blk_mq_start_request from the core blk-mq code before calling into ->queue_rq there is a racy window where the timeout handler can hit before we've fully set up the driver specific part of the command. Move the call to blk_mq_start_request into the driver so the driver can start the request only once it is fully set up. Using my original (newer) machine with a SAS SSD, today I'm seeing only the "blown stack" oops on umount. And on the next reboot, if use_blk_mq=Y then doing the mount (on the SAS SSD) causes an instant reboot. Same with and without this patch. I'll try again with the SATA SSD (but I need to archive its contents first) and maybe I can get back to the scsi_times_out oops. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Block/SCSI data integrity update v3
> "Jens" == Jens Axboe writes: Jens> Thanks, I'll do that once Martin respins the previous series (was Jens> pulled due to multiple issues). I bumped bi_rw to u64 as discussed. Will get that into -next to see if there's any fallout. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/14] sd: Honor block layer integrity handling flags
> "Sagi" == Sagi Grimberg writes: [Back from vacation] >> + [SCSI_PROT_WRITE_PASS] = SCSI_PROT_TRANSFER_PI | >> + SCSI_PROT_GUARD_CHECK | >> + SCSI_PROT_REF_CHECK | >> + SCSI_PROT_REF_INCREMENT | >> + SCSI_PROT_IP_CHECKSUM, Sagi> A bit strange to me that you put REF_CHECK & REF_INCREMENT flag Sagi> depending on the prot_op while it really depends on the prot_type. It doesn't just depend on the prot_type. You need to be able to tell the HBA to increment/test the ref tag even for a Type 0 device. Sagi> I have a question here, What are your plans with explicit ref/app Sagi> escape flags? Were these just left behind on this series? We don't yet have a need for them in Linux so I didn't define them. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
On Thu, Sep 11, 2014 at 3:02 AM, Christoph Hellwig wrote: > On Wed, Sep 10, 2014 at 09:40:11AM +0800, Ming Lei wrote: >> I am wondering we can do that because lifetime is totally different >> between flush requests and tag_set requests which are initialized >> before request queue is created. > > We shouldn't do it in the tag sets, but where we allocate and free > each hctx: blk_mq_init_queue and blk_mq_free_hw_queues. That should work, but both flush queue's allocation and .init_request() have to move to the function because hctx->numa_node is basically ready in blk_mq_init_queue(). Then blk_init_flush() only need to allocate the data for legacy case. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM
Tom, thanks for reviewing this! My reply below! On Tue, Sep 9, 2014 at 11:46 PM, Tom Gundersen wrote: > On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez > wrote: >> On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley >> wrote: >>> On Tue, 2014-09-09 at 12:16 -0700, Luis R. Rodriguez wrote: On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley wrote: > If we want to sort out some sync/async mechanism for probing devices, as > an agreement between the init systems and the kernel, that's fine, but > its a to-be negotiated enhancement. Unfortunately as Tejun notes the train has left which already made assumptions on this. >>> >>> Well, that's why it's a bug. It's a material regression impacting >>> users. >> >> Indeed. I believe the issue with this regression however was that the >> original commit e64fae55 (January 2012) was only accepted by *kernel >> folks* to be a real regression until recently. > > Just for the record, this only caused user-visible problems after > kernel commit 786235ee (November 2013), right? Another one was cxgb4: https://bugzilla.novell.com/show_bug.cgi?id=877622 SLE12 does not yet have commit 786235ee merged. Benjamim did some hard work to debug this and trace the kill down to systemd-udev. A debug kernel build has been provided now to try to pick up exactly on the place where the kill was received, but its at least clear this came from systemd. >> More than two years >> have gone by on growing design and assumptions on top of that original >> commit. I'm not sure if *systemd folks* yet believe its was a design >> regression? > > I don't think so. udev should not allow its workers to run for an > unbounded length of time. Whether the upper bound should be 30, 60, > 180 seconds or something else is up for debate (currently it is 60, > but if that is too short for some drivers we could certainly revisit > that). That's the thing -- the timeout was put in place under the assumption probe was asyncronous and its not, the driver core issues both module init *and* probe together, the loader has to wait. That alone makes the timeout a design flaw, and then systemd carried on top of that design over two years after that. Its not systemd's fault, its just that we never spoke about this as a design thing broadly and we should have, and I will mention that even when the first issues creeped up, the issue was still tossed back a driver problems. It was only until recently that we realized that both init and probe run together that we've been thinking about this problem differently. Systemd was trying to ensure init on driver don't take long but its not init that is taking long, its probe, and probe gets then penalized as the driver core batches both init and probe synchronously before finishing module loading. Furthermore as clarified by Tejun random userland is known to exist that will wait indefinitely for module loading under the simple assumption things *are done synchronously*, and its precisely why we can't just blindly enable async probe upstream through a new driver boolean as it can be unfair to this old userland. What is being evaluated is to enable aync probe for *all* drivers through a new general system-wide option. We cannot regress old userspace and assumptions but we can create a new shiny universe. > Moreover, it seems from this discussion that the aim is (still) > that insmod should be near-instantaneous (i.e., not wait for probe), The only reason that is being discussed is that systemd has not accepted the timeout as a system design despite me pointing out the original design flaw recently and at this point even if was accepted as a design flaw it seems its too late. The approach taken to help make all drivers async is simply an afterthought to give systemd what it *thought* was in place, and it by no measure should be considered the proper fix to the regression introduced by systemd, it may perhaps be the right step long term for systemd systems given it goes with what it assumed was there, but the timeout was flawed. Its not clear if systemd can help with old kernels, it seems the ship has sailed and there seems no options but for folks to work around that -- unless of course some reasonable solution is found which doesn't break current systemd design? > so it seems to me that the basic design is correct and all we need is > some temporary work-around and a way to better detect misbehaving > drivers? As part of this series I addressed hunting for the "misbehaving drivers" in-kernel as I saw no progress on the systemd side of things to non-fatally detect "misbehaving drivers" despite my original RFCs and request for advice. I quote "misbehaving drivers" as its a flawed view to consider them misbehaving now in light of clarifications of how the driver core works in that it batches both init and probe together always and we can't be penalizing long probes due to the fact long probes are simply fine. My patch to pick up "misbeha
Re: Updated linux uas driver, please test
On 14-09-10 08:13 AM, Hans de Goede wrote: Hi All, I'm mailing all of you because you've reported various problems with the new uas support in kernel 3.16 and later. I've been working on making the uas driver more resilient to errors, as well as improved logging so we can easier figure out the cause of errors. I would like to ask you all to test a standalone version of the new uas driver with the devices you've been having trouble with before, and report the results to me. Testing instructions: 1) Remove any usb-storage.quirks= setting from the kernel commandline, and remove any /etc/modprobe.conf* files doing the same, boot your machine without the uas device attached. 2) Make sure your machine is set up for building kernel modules, usually this means installing kernel-devel and gcc packages, see your distributions documentation for more info 3) Download all files from here: https://fedorapeople.org/~jwrdegoede/uas/ And put them all in a single directory, named e.g. uas 4) Start a terminal, cd into the uas directory 5) Run the following commands: make sudo rmmod uas sudo insmod ./uas.ko 6) Connect your uas device 7) Wait for the disk to show up (wait circa 1 minute max), then do: dmesg > dmesg.log lsusb -v > lsusb.log 8) Test the uas disk Once done please send me a mail, in this mail please 1) Describe how the disk worked, did it show up in a reasonable time, and did it work? 2) Attach dmesg.log and lsusb.log Could you give some sort of indication of the dd throughput time (on READs) with UAS given a recent SATA SSD that can source data faster than 300 MB/sec (say)? My ASM1051 based dock under W7 got an underwhelming maximum of 42 MB/sec with such a SATA SSD. And I saw enough stupid meta-data from SCSI commands to suggest that product should be binned. Doug Gilbert -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
On Wed, Sep 10, 2014 at 09:40:11AM +0800, Ming Lei wrote: > I am wondering we can do that because lifetime is totally different > between flush requests and tag_set requests which are initialized > before request queue is created. We shouldn't do it in the tag sets, but where we allocate and free each hctx: blk_mq_init_queue and blk_mq_free_hw_queues. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 01/16] scsi: support well known logical units
On Wed, Sep 10, 2014 at 02:54:08PM +0300, Dolev Raviv wrote: > From: Subhash Jadavani > > REPORT LUNS command has "SELECT REPORT" field which controls what type of > logical units to be reported by device server. According to UFS device > standard, if this field is set to 0, REPORT LUNS would report only report > standard logical units. If it's set to 1 then it would report only well > known logical unit and if it's set to 2 then device would report both > standard and well known logical units. This is the defintion of the field in SPC (does the UFS spec duplicate all of SPC?), but still doesn't explain why you want it. Also the SELECT REPORT field only appeared in SPC-3, so we should not set it for devices that report older standards compliance. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On 09/10/2014 12:40 PM, Christoph Hellwig wrote: > On Wed, Sep 10, 2014 at 12:26:57PM -0600, Jens Axboe wrote: >>> I have to. It's set by start_request, so we need to pass down the last >>> argument to keep the old behavior. And once we pass the argument we >>> can just it directly. >> >> It could still be done in the caller, but arguably, you'd have to do it >> twice unless the ->queue_rq() call was rolled into a function. > > At which point we'd better do it the right way and just pass the flag > instead of wasting a request flag for it. The other benefit is that > there is a clear compile time API break for ->queue_rq that reminds > people that need to start using blk_mq_start_request. Yeah that's a good point, hopefully they will look up the commit and figure out what to do. I'm not adverse to doing these two things at once, but it should at least have a good mention of it in the changelog. It's not even mentioned. > Anyway, still waiting for test reports of the people that can reproduce > the timeouts. By the time I'll submit the patch it should have a much > better changelog. Thanks, sounds good. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On Wed, Sep 10, 2014 at 12:26:57PM -0600, Jens Axboe wrote: > > I have to. It's set by start_request, so we need to pass down the last > > argument to keep the old behavior. And once we pass the argument we > > can just it directly. > > It could still be done in the caller, but arguably, you'd have to do it > twice unless the ->queue_rq() call was rolled into a function. At which point we'd better do it the right way and just pass the flag instead of wasting a request flag for it. The other benefit is that there is a clear compile time API break for ->queue_rq that reminds people that need to start using blk_mq_start_request. Anyway, still waiting for test reports of the people that can reproduce the timeouts. By the time I'll submit the patch it should have a much better changelog. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On 09/10/2014 12:09 PM, Christoph Hellwig wrote: > On Wed, Sep 10, 2014 at 10:47:49AM -0600, Jens Axboe wrote: >> BTW, please don't mix up the REQ_END and ->queue_rq() changes with the >> changed start_request API. > > I have to. It's set by start_request, so we need to pass down the last > argument to keep the old behavior. And once we pass the argument we > can just it directly. It could still be done in the caller, but arguably, you'd have to do it twice unless the ->queue_rq() call was rolled into a function. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On Wed, Sep 10, 2014 at 10:47:49AM -0600, Jens Axboe wrote: > BTW, please don't mix up the REQ_END and ->queue_rq() changes with the > changed start_request API. I have to. It's set by start_request, so we need to pass down the last argument to keep the old behavior. And once we pass the argument we can just it directly. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/21] uas: Use scsi_print_command
Hi Robert, On 09/10/2014 06:08 PM, Elliott, Robert (Server Storage) wrote: > > >> -Original Message- >> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- >> ow...@vger.kernel.org] On Behalf Of Hans de Goede >> Sent: Wednesday, 10 September, 2014 6:47 AM >> To: Greg Kroah-Hartman >> Cc: linux-...@vger.kernel.org; linux-scsi@vger.kernel.org; >> sta...@vger.kernel.org; Hans de Goede >> Subject: [PATCH 18/21] uas: Use scsi_print_command >> >> Use scsi_print_command to print commands during errors, rather then >> printing >> the rather meaningless pointer to the command. >> >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/storage/uas.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 46b8788..220f4c7 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -229,8 +229,8 @@ static void uas_log_cmd_state(struct scsi_cmnd >> *cmnd, const char *caller) >> struct uas_cmd_info *ci = (void *)&cmnd->SCp; >> >> scmd_printk(KERN_INFO, cmnd, >> -"%s %p tag %d, >> inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s\n", >> -caller, cmnd, uas_get_tag(cmnd), >> +"%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ", >> +caller, uas_get_tag(cmnd), >> (ci->state & SUBMIT_STATUS_URB) ? " s-st" : "", >> (ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "", >> (ci->state & SUBMIT_DATA_IN_URB)? " s-in" : "", >> @@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd >> *cmnd, const char *caller) >> (ci->state & COMMAND_COMPLETED) ? " done" : "", >> (ci->state & COMMAND_ABORTED) ? " abort" : "", >> (ci->state & IS_IN_WORK_LIST) ? " work" : ""); >> +scsi_print_command(cmnd); >> } > > The SCSI midlayer already does a scsi_print_command, under > the MLCOMPLETE logging level. Printing the command in > the LLD is redundant; Printing the scmd pointer was not, > as it matched the prints in the SCSI midlayer, letting > you find all the messages for a command. > > In Hannes' logging patch series, Christoph Hellwig suggested > printing the block layer tag instead of the scmd pointer. > If UAS uses that tag directly, then the uas_get_tag() print > already does that. If they're not identical (looks like > they differ by 2), then you might want to print both. > > More general comment: > scsi-mq stress testing has shown that calling printk > on each command with an error is problematic. If the > system runs into hundreds or thousands of errors, the > printks take so long they induce more timeouts and errors. > > As discussed in the threads by Yoshihiro YUNOMAE and Hannes > Reinecke, you might want to: > * tuck the scmd_printk inside SCSI_LOG_LLCOMPLETE so > the user can control the verbosity > * use a _ratelimited version of each print > * plan to add and eventually switch to tracepoints, so > logging doesn't have performance impacts Thanks for your feedback, indeed we may need to cut down on the logging in the future. ATM our focus is mostly on getting the UAS driver work reliable with all the (cheap) uas hardware out there. Most errors we are getting are not disk errors, but errors due to the usb <-> sata bridge chip not groking some command. Being able to directly see the command which is tripping up the bridge in a dmesg output without needing to tell the user to take extra action is very helpful at this point in the uas' driver development. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
On 2014-09-10 09:41, Christoph Hellwig wrote: While it might not help with a blown stack, can you give the patch below a try? I tries to solve a problem where the timeout handler hits before we've fully set up a command. While I'd like to understand the root cause of why we're hitting it as well, I'd also really to fix that race. It would also be good to get a gdb listing of the exact area in scsi_times_out listed in the oops. It's a really long window, but it does exist. I'd be curious if the patch makes a difference for this weird case. I have not seen anything like it here. BTW, please don't mix up the REQ_END and ->queue_rq() changes with the changed start_request API. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/21] uas: Use scsi_print_command
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- > ow...@vger.kernel.org] On Behalf Of Hans de Goede > Sent: Wednesday, 10 September, 2014 6:47 AM > To: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org; linux-scsi@vger.kernel.org; > sta...@vger.kernel.org; Hans de Goede > Subject: [PATCH 18/21] uas: Use scsi_print_command > > Use scsi_print_command to print commands during errors, rather then > printing > the rather meaningless pointer to the command. > > Signed-off-by: Hans de Goede > --- > drivers/usb/storage/uas.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 46b8788..220f4c7 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -229,8 +229,8 @@ static void uas_log_cmd_state(struct scsi_cmnd > *cmnd, const char *caller) > struct uas_cmd_info *ci = (void *)&cmnd->SCp; > > scmd_printk(KERN_INFO, cmnd, > - "%s %p tag %d, > inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s\n", > - caller, cmnd, uas_get_tag(cmnd), > + "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ", > + caller, uas_get_tag(cmnd), > (ci->state & SUBMIT_STATUS_URB) ? " s-st" : "", > (ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "", > (ci->state & SUBMIT_DATA_IN_URB)? " s-in" : "", > @@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd > *cmnd, const char *caller) > (ci->state & COMMAND_COMPLETED) ? " done" : "", > (ci->state & COMMAND_ABORTED) ? " abort" : "", > (ci->state & IS_IN_WORK_LIST) ? " work" : ""); > + scsi_print_command(cmnd); > } The SCSI midlayer already does a scsi_print_command, under the MLCOMPLETE logging level. Printing the command in the LLD is redundant; Printing the scmd pointer was not, as it matched the prints in the SCSI midlayer, letting you find all the messages for a command. In Hannes' logging patch series, Christoph Hellwig suggested printing the block layer tag instead of the scmd pointer. If UAS uses that tag directly, then the uas_get_tag() print already does that. If they're not identical (looks like they differ by 2), then you might want to print both. More general comment: scsi-mq stress testing has shown that calling printk on each command with an error is problematic. If the system runs into hundreds or thousands of errors, the printks take so long they induce more timeouts and errors. As discussed in the threads by Yoshihiro YUNOMAE and Hannes Reinecke, you might want to: * tuck the scmd_printk inside SCSI_LOG_LLCOMPLETE so the user can control the verbosity * use a _ratelimited version of each print * plan to add and eventually switch to tracepoints, so logging doesn't have performance impacts --- Rob ElliottHP Server Storage -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature support
On Wed, Sep 10, 2014 at 05:28:40PM +0200, Tomas Henzl wrote:
> On 09/10/2014 05:06 PM, Elliott, Robert (Server Storage) wrote:
> >> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> >> ow...@vger.kernel.org] On Behalf Of Sumit Saxena
> >>
> >>> From: Tomas Henzl [mailto:the...@redhat.com]
> >>>
> >>> With several controllers in a system this may take a lot memory,
> >>> could you also in case when a kdump kernel is running lower it,
> >>> by not using this feature?
> >>>
> >> Agreed, we will disable this feature for kdump kernel by adding
> >> "reset_devices" global varaiable.
> >> That check is required for only one place, throughout the code, this
> >> feature will remain disabled. Code snippet for the same-
> >>
> >> instance->crash_dump_drv_support = (!reset_devices) &&
> >> crashdump_enable &&
> >> instance->crash_dump_fw_support &&
> >> instance->crash_dump_buf);
> >> if(instance->crash_dump_drv_support) {
> >> printk(KERN_INFO "megaraid_sas: FW Crash dump is
> >> supported\n");
> >> megasas_set_crash_dump_params(instance,
> >> MR_CRASH_BUF_TURN_OFF);
> >>
> >> } else {
> >> ..
> >> }
> > Network drivers have been running into similar problems.
> >
> > There's a new patch from Amir coming through net-next to make
> > is_kdump_kernel() (in crash_dump.h) accessible to modules.
> > That may be a better signal than reset_devices that the
> > driver should use minimal resources.
> >
> > http://comments.gmane.org/gmane.linux.network/324737
> >
> > I'm not sure about the logistics of a SCSI patch depending
> > on a net-next patch.
>
> Probably better to start with reset_devices and switch to is_kdump_kernel()
> later.
> This is not a discussion about reset_devices versus is_kdump_kernel, but
> while it looks good to have it distinguished - is the reset_devices actually
> used anywhere else than in kdump kernel?
I think usage of reset_devices for lowering memory footprint of driver
is plain wrong. It tells driver to only reset the device as BIOS might
not have done it right or we skipped BIOS completely.
Using is_kdump_kernel() is also not perfect either but atleast better than
reset_devices.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing
On Wed, Sep 10, 2014 at 09:21:24AM +0200, Hans de Goede wrote:
> I've applied the patch, this results in the following new dmesg output
> when using uas:
>
> [ 120.602632] initialized host-wide tag map!
>
> Thank you for looking into this.
So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
up. I can't really come up with a good explanation for it, but there
even without that there is an elephant in the room: as part of the
scsi-mq series I moved the bqt field used for this into a union with the
new blk_mq_tag_set. Below is a patch to get rid of that union, can you
try if that fixes it?
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index d0f69a3..bc2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -584,10 +584,8 @@ struct Scsi_Host {
* Area to keep a shared tag map (if needed, will be
* NULL if not).
*/
- union {
- struct blk_queue_tag*bqt;
- struct blk_mq_tag_set tag_set;
- };
+ struct blk_queue_tag*bqt;
+ struct blk_mq_tag_set tag_set;
atomic_t host_busy;/* commands actually active on
low-level */
atomic_t host_blocked;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lk 3.17-rc4 blk_mq large write problems
While it might not help with a blown stack, can you give the patch below
a try? I tries to solve a problem where the timeout handler hits
before we've fully set up a command. While I'd like to understand the
root cause of why we're hitting it as well, I'd also really to fix that
race. It would also be good to get a gdb listing of the exact area in
scsi_times_out listed in the oops.
---
From: Christoph Hellwig
Subject: blk-mq: call blk_mq_start_request from ->queue_rq
When we call blk_mq_start_request from the core blk-mq code before calling into
->queue_rq there is a racy window where the timeout handler can hit before we've
fully set up the driver specific part of the command.
Move the call to blk_mq_start_request into the driver so the driver can start
the request only once it is fully set up.
Signed-off-by: Christoph Hellwig
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5189cb1..db9990b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -374,7 +374,7 @@ void blk_mq_complete_request(struct request *rq)
}
EXPORT_SYMBOL(blk_mq_complete_request);
-static void blk_mq_start_request(struct request *rq, bool last)
+void blk_mq_start_request(struct request *rq)
{
struct request_queue *q = rq->q;
@@ -405,29 +405,18 @@ static void blk_mq_start_request(struct request *rq, bool
last)
*/
rq->nr_phys_segments++;
}
-
- /*
-* Flag the last request in the series so that drivers know when IO
-* should be kicked off, if they don't do it on a per-request basis.
-*
-* Note: the flag isn't the only condition drivers should do kick off.
-* If drive is busy, the last request might not have the bit set.
-*/
- if (last)
- rq->cmd_flags |= REQ_END;
}
+EXPORT_SYMBOL(blk_mq_start_request);
static void __blk_mq_requeue_request(struct request *rq)
{
struct request_queue *q = rq->q;
trace_block_rq_requeue(q, rq);
- clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
-
- rq->cmd_flags &= ~REQ_END;
-
- if (q->dma_drain_size && blk_rq_bytes(rq))
- rq->nr_phys_segments--;
+ if (test_and_clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) {
+ if (q->dma_drain_size && blk_rq_bytes(rq))
+ rq->nr_phys_segments--;
+ }
}
void blk_mq_requeue_request(struct request *rq)
@@ -735,9 +724,7 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx
*hctx)
rq = list_first_entry(&rq_list, struct request, queuelist);
list_del_init(&rq->queuelist);
- blk_mq_start_request(rq, list_empty(&rq_list));
-
- ret = q->mq_ops->queue_rq(hctx, rq);
+ ret = q->mq_ops->queue_rq(hctx, rq, list_empty(&rq_list));
switch (ret) {
case BLK_MQ_RQ_QUEUE_OK:
queued++;
@@ -1177,14 +1164,13 @@ static void blk_mq_make_request(struct request_queue
*q, struct bio *bio)
int ret;
blk_mq_bio_to_request(rq, bio);
- blk_mq_start_request(rq, true);
/*
* For OK queue, we are done. For error, kill it. Any other
* error (busy), just add it to our list as we previously
* would have done
*/
- ret = q->mq_ops->queue_rq(data.hctx, rq);
+ ret = q->mq_ops->queue_rq(data.hctx, rq, true);
if (ret == BLK_MQ_RQ_QUEUE_OK)
goto done;
else {
diff --git a/drivers/block/mtip32xx/mtip32xx.c
b/drivers/block/mtip32xx/mtip32xx.c
index db1e956..9b0127a 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3775,13 +3775,16 @@ static bool mtip_check_unal_depth(struct blk_mq_hw_ctx
*hctx,
return false;
}
-static int mtip_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *rq)
+static int mtip_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *rq,
+ bool last)
{
int ret;
if (unlikely(mtip_check_unal_depth(hctx, rq)))
return BLK_MQ_RQ_QUEUE_BUSY;
+ blk_mq_start_request(rq);
+
ret = mtip_submit_request(hctx, rq);
if (likely(!ret))
return BLK_MQ_RQ_QUEUE_OK;
diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index a3b042c..d098adfbb 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -313,10 +313,13 @@ static void null_request_fn(struct request_queue *q)
}
}
-static int null_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *rq)
+static int null_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *rq,
+ bool last)
{
struct nullb_cmd *cmd = blk_mq_rq_to_pdu(rq);
+ blk_mq_start_request(rq);
+
cmd->rq = rq;
cmd->nq = hctx->driver_data;
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.
Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature support
On 09/10/2014 05:06 PM, Elliott, Robert (Server Storage) wrote:
>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>> ow...@vger.kernel.org] On Behalf Of Sumit Saxena
>>
>>> From: Tomas Henzl [mailto:the...@redhat.com]
>>>
>>> With several controllers in a system this may take a lot memory,
>>> could you also in case when a kdump kernel is running lower it,
>>> by not using this feature?
>>>
>> Agreed, we will disable this feature for kdump kernel by adding
>> "reset_devices" global varaiable.
>> That check is required for only one place, throughout the code, this
>> feature will remain disabled. Code snippet for the same-
>>
>> instance->crash_dump_drv_support = (!reset_devices) &&
>> crashdump_enable &&
>> instance->crash_dump_fw_support &&
>> instance->crash_dump_buf);
>> if(instance->crash_dump_drv_support) {
>> printk(KERN_INFO "megaraid_sas: FW Crash dump is
>> supported\n");
>> megasas_set_crash_dump_params(instance,
>> MR_CRASH_BUF_TURN_OFF);
>>
>> } else {
>> ..
>> }
> Network drivers have been running into similar problems.
>
> There's a new patch from Amir coming through net-next to make
> is_kdump_kernel() (in crash_dump.h) accessible to modules.
> That may be a better signal than reset_devices that the
> driver should use minimal resources.
>
> http://comments.gmane.org/gmane.linux.network/324737
>
> I'm not sure about the logistics of a SCSI patch depending
> on a net-next patch.
Probably better to start with reset_devices and switch to is_kdump_kernel()
later.
This is not a discussion about reset_devices versus is_kdump_kernel, but
while it looks good to have it distinguished - is the reset_devices actually
used anywhere else than in kdump kernel?
>
> ---
> Rob ElliottHP Server Storage
>
>
>
>
> N�r��y���b�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
> ���j:+v���w�j�mzZ+�ݢj"��!tml=
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] megaraid_sas : MFI MPT linked list corruption fix
On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
> Problem statement:
> MFI link list in megaraid_sas driver is used from mfi-mpt pass-through
> commands.
> This list can be corrupted due to many possible race conditions in driver and
> eventually we may see kernel panic.
>
> One example -
> MFI frame is freed from calling process as driver send command via polling
> method and interrupt
> for that command comes after driver free mfi frame (actually even after some
> other context reuse
> the mfi frame). When driver receive MPT frame in ISR, driver will be using
> the index of MFI and
> access that MFI frame and finally in-used MFI frame’s list will be corrupted.
>
> High level description of new solution -
> Free MFI and MPT command from same context.
> Free both the command either from process (from where mfi-mpt pass-through
> was called) or from
> ISR context. Do not split freeing of MFI and MPT, because it creates the race
> condition which
> will do MFI/MPT list corruption.
>
> Renamed the cmd_pool_lock which is used in instance as well as fusion with
> below name.
> mfi_pool_lock and mpt_pool_lock to add more code readability.
>
> Signed-off-by: Sumit Saxena
> Signed-off-by: Kashyap Desai
> ---
> drivers/scsi/megaraid/megaraid_sas.h| 25 +++-
> drivers/scsi/megaraid/megaraid_sas_base.c | 196
>
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 95 ++
> drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 +-
> 4 files changed, 235 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> b/drivers/scsi/megaraid/megaraid_sas.h
> index 156d4b9..f99db18 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -1016,6 +1016,12 @@ struct megasas_ctrl_info {
>
> #define VD_EXT_DEBUG 0
>
> +enum MR_MFI_MPT_PTHR_FLAGS {
> + MFI_MPT_DETACHED = 0,
> + MFI_LIST_ADDED = 1,
> + MFI_MPT_ATTACHED = 2,
> +};
> +
> /* Frame Type */
> #define IO_FRAME 0
> #define PTHRU_FRAME 1
> @@ -1033,7 +1039,7 @@ struct megasas_ctrl_info {
> #define MEGASAS_IOCTL_CMD0
> #define MEGASAS_DEFAULT_CMD_TIMEOUT 90
> #define MEGASAS_THROTTLE_QUEUE_DEPTH 16
> -
> +#define MEGASAS_BLOCKED_CMD_TIMEOUT 60
> /*
> * FW reports the maximum of number of commands that it can accept (maximum
> * commands that can be outstanding) at any time. The driver must report a
> @@ -1652,7 +1658,7 @@ struct megasas_instance {
> struct megasas_cmd **cmd_list;
> struct list_head cmd_pool;
> /* used to sync fire the cmd to fw */
> - spinlock_t cmd_pool_lock;
> + spinlock_t mfi_pool_lock;
> /* used to sync fire the cmd to fw */
> spinlock_t hba_lock;
> /* used to synch producer, consumer ptrs in dpc */
> @@ -1839,6 +1845,11 @@ struct megasas_cmd {
>
> struct list_head list;
> struct scsi_cmnd *scmd;
> +
> + void *mpt_pthr_cmd_blocked;
> + atomic_t mfi_mpt_pthr;
> + u8 is_wait_event;
> +
> struct megasas_instance *instance;
> union {
> struct {
> @@ -1927,4 +1938,14 @@ int megasas_set_crash_dump_params(struct
> megasas_instance *instance,
> void megasas_free_host_crash_buffer(struct megasas_instance *instance);
> void megasas_fusion_crash_dump_wq(struct work_struct *work);
>
> +void megasas_return_cmd_fusion(struct megasas_instance *instance,
> + struct megasas_cmd_fusion *cmd);
> +int megasas_issue_blocked_cmd(struct megasas_instance *instance,
> + struct megasas_cmd *cmd, int timeout);
> +void __megasas_return_cmd(struct megasas_instance *instance,
> + struct megasas_cmd *cmd);
> +
> +void megasas_return_mfi_mpt_pthr(struct megasas_instance *instance,
> + struct megasas_cmd *cmd_mfi, struct megasas_cmd_fusion *cmd_fusion);
> +
> #endif /*LSI_MEGARAID_SAS_H */
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 086beee..50d69eb 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -210,43 +210,66 @@ struct megasas_cmd *megasas_get_cmd(struct
> megasas_instance
> unsigned long flags;
> struct megasas_cmd *cmd = NULL;
>
> - spin_lock_irqsave(&instance->cmd_pool_lock, flags);
> + spin_lock_irqsave(&instance->mfi_pool_lock, flags);
>
> if (!list_empty(&instance->cmd_pool)) {
> cmd = list_entry((&instance->cmd_pool)->next,
>struct megasas_cmd, list);
> list_del_init(&cmd->list);
> + atomic_set(&cmd->mfi_mpt_pthr, MFI_MPT_DETACHED);
> } else {
> printk(KERN_ERR "megasas: Command pool empty!\n");
> }
>
> - spin_unlock_irqrestore(&instance->cmd_pool_lock, flags);
> + spin_unlock_irqrestore(&instance->m
RE: [PATCH 04/11] megaraid_sas : Firmware crash dump feature support
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Sumit Saxena
>
> >From: Tomas Henzl [mailto:the...@redhat.com]
> >
> >With several controllers in a system this may take a lot memory,
> > could you also in case when a kdump kernel is running lower it,
> > by not using this feature?
> >
> Agreed, we will disable this feature for kdump kernel by adding
> "reset_devices" global varaiable.
> That check is required for only one place, throughout the code, this
> feature will remain disabled. Code snippet for the same-
>
> instance->crash_dump_drv_support = (!reset_devices) &&
> crashdump_enable &&
> instance->crash_dump_fw_support &&
> instance->crash_dump_buf);
> if(instance->crash_dump_drv_support) {
> printk(KERN_INFO "megaraid_sas: FW Crash dump is
> supported\n");
> megasas_set_crash_dump_params(instance,
> MR_CRASH_BUF_TURN_OFF);
>
> } else {
> ..
> }
Network drivers have been running into similar problems.
There's a new patch from Amir coming through net-next to make
is_kdump_kernel() (in crash_dump.h) accessible to modules.
That may be a better signal than reset_devices that the
driver should use minimal resources.
http://comments.gmane.org/gmane.linux.network/324737
I'm not sure about the logistics of a SCSI patch depending
on a net-next patch.
---
Rob ElliottHP Server Storage
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
Hi, On 09/10/2014 04:38 PM, Peter Hurley wrote: > [ +cc Peter Zijlstra, Ingo Molnar ] > > On 09/10/2014 07:46 AM, Hans de Goede wrote: >> From: Sanjeev Sharma >> >> On some architecture spin_is_locked() always return false in >> uniprocessor configuration and therefore it would be advise to replace >> with lockdep_assert_held(). >> >> Signed-off-by: Sanjeev Sharma >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/storage/uas.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 3f42785..05b2d8e 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info >> *devinfo, >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp); >> >> uas_log_cmd_state(cmnd, caller); >> -WARN_ON_ONCE(!spin_is_locked(&devinfo->lock)); >> +lockdep_assert_held(&devinfo->lock); > > This change isn't equivalent. > > lockdep_assert_held() will continue to emit warnings; ie., there is no > "once" functionality. Same for the other changes below. Given that these should really never ever happen, that is not really a problem IMHO. The idea ws to replace the wrong use of spin_is_locked with some other sanity check, preferably a light weight one, the once behavior is not that important. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] megaraid_sas : Extended VD support
On Wed, Sep 10, 2014 at 7:20 PM, Tomas Henzl wrote:
>
> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
> > Current MegaRAID firmware and hence the driver only supported 64VDs.
> > E.g: If the user wants to create more than 64VD on a controller,
> > it is not possible on current firmware/driver.
> >
> > New feature and requirement to support upto 256VD, firmware/driver/apps
> > need changes.
> > In addition to that there must be a backward compatibility of the new
> > driver with the
> > older firmware and vice versa.
> >
> > RAID map is the interface between Driver and FW to fetch all required
> > fields(attributes) for each Virtual Drives.
> > In the earlier design driver was using the FW copy of RAID map where as
> > in the new design the Driver will keep the RAID map copy of its own; on
> > which
> > it will operate for any raid map access in fast path.
> >
> > Local driver raid map copy will provide ease of access through out the code
> > and provide generic interface for future FW raid map changes.
> >
> > For the backward compatibility driver will notify FW that it supports 256VD
> > to the FW in driver capability field.
> > Based on the controller properly returned by the FW, the Driver will know
> > whether it supports 256VD or not and will copy the RAID map accordingly.
> >
> > At any given time, driver will always have old or new Raid map.
> > So with this changes, driver can also work in host lock less mode. Please
> > see next patch which enable host lock less mode for megaraid_sas driver.
> >
> > Signed-off-by: Sumit Saxena
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas.h| 73 +++---
> > drivers/scsi/megaraid/megaraid_sas_base.c | 205
> >
> > drivers/scsi/megaraid/megaraid_sas_fp.c | 195
> > ++
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 118
> > drivers/scsi/megaraid/megaraid_sas_fusion.h | 95 -
> > 5 files changed, 502 insertions(+), 184 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index e0f03e2..5dedf09 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -390,7 +390,6 @@ enum MR_LD_QUERY_TYPE {
> > #define MR_EVT_FOREIGN_CFG_IMPORTED 0x00db
> > #define MR_EVT_LD_OFFLINE 0x00fc
> > #define MR_EVT_CTRL_HOST_BUS_SCAN_REQUESTED 0x0152
> > -#define MAX_LOGICAL_DRIVES 64
> >
> > enum MR_PD_STATE {
> > MR_PD_STATE_UNCONFIGURED_GOOD = 0x00,
> > @@ -468,14 +467,14 @@ struct MR_LD_LIST {
> > u8 state;
> > u8 reserved[3];
> > u64 size;
> > - } ldList[MAX_LOGICAL_DRIVES];
> > + } ldList[MAX_LOGICAL_DRIVES_EXT];
> > } __packed;
> >
> > struct MR_LD_TARGETID_LIST {
> > u32 size;
> > u32 count;
> > u8 pad[3];
> > - u8 targetId[MAX_LOGICAL_DRIVES];
> > + u8 targetId[MAX_LOGICAL_DRIVES_EXT];
> > };
> >
> >
> > @@ -941,6 +940,15 @@ struct megasas_ctrl_info {
> > * HA cluster information
> > */
> > struct {
> > +#if defined(__BIG_ENDIAN_BITFIELD)
> > + u32 reserved:26;
> > + u32 premiumFeatureMismatch:1;
> > + u32 ctrlPropIncompatible:1;
> > + u32 fwVersionMismatch:1;
> > + u32 hwIncompatible:1;
> > + u32 peerIsIncompatible:1;
> > + u32 peerIsPresent:1;
> > +#else
> > u32 peerIsPresent:1;
> > u32 peerIsIncompatible:1;
> > u32 hwIncompatible:1;
> > @@ -948,6 +956,7 @@ struct megasas_ctrl_info {
> > u32 ctrlPropIncompatible:1;
> > u32 premiumFeatureMismatch:1;
> > u32 reserved:26;
> > +#endif
> > } cluster;
> >
> > char clusterId[16]; /*7D4h */
> > @@ -962,9 +971,17 @@ struct megasas_ctrl_info {
> > #if defined(__BIG_ENDIAN_BITFIELD)
> > u32 reserved:25;
> > u32 supportCrashDump:1;
> > - u32 reserved1:6;
> > + u32 supportMaxExtLDs:1;
> > + u32 supportT10RebuildAssist:1;
> > + u32 supportDisableImmediateIO:1;
> > + u32 supportThermalPollInterval:1;
> > + u32 supportPersonalityChange:2;
> > #else
> > - u32 reserved1:6;
> > + u32 supportPersonalityChange:2;
> > + u32 supportThermalPollInterval:1;
> > + u32 supportDisableImmediateIO:1;
> > + u32 supportT10RebuildAssist:1;
> > + u32 supportMaxExtLDs:1;
> > u32 supportCrashDump:1;
> > u32 reserved:25;
> > #endif
> > @@ -979,
Re: [PATCH 08/11] megaraid_sas : Add module parameter to disable IRQ-CPU affinity hint
On Wed, Sep 10, 2014 at 7:46 PM, Tomas Henzl wrote:
> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>> For certain deployment, we may need to disable irq cpu affinity hint.
>> This module parameter provides option for use to disable irq cpu affinity
>> hint
>> and allow irqbalancer to handle the rest.
>
> Only curious , in which environments causes this which issues?
For few deployment cases where CPU is managed via Cgroup and user
knows how they are going to use their system resources.
For those cases driver hinting may not be useful. For simplicity
customer might like this type of module parameter.
Kashyap
>
> Thanks,
> Tomas
>
>>
>> Signed-off-by: Sumit Saxena
>> Signed-off-by: Kashyap Desai
>> ---
>> drivers/scsi/megaraid/megaraid_sas_base.c | 60
>> +++
>> 1 file changed, 38 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>> b/drivers/scsi/megaraid/megaraid_sas_base.c
>> index 07255c1..086beee 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>> @@ -89,6 +89,10 @@ module_param(resetwaittime, int, S_IRUGO);
>> MODULE_PARM_DESC(resetwaittime, "Wait time in seconds after I/O timeout "
>>"before resetting adapter. Default: 180");
>>
>> +int smp_affinity_enable = 1;
>> +module_param(smp_affinity_enable, int, S_IRUGO);
>> +MODULE_PARM_DESC(smp_affinity_enable, "SMP affinity feature enable/disbale
>> Default: enable(1)");
>> +
>> MODULE_LICENSE("GPL");
>> MODULE_VERSION(MEGASAS_VERSION);
>> MODULE_AUTHOR("megaraidli...@lsi.com");
>> @@ -5160,8 +5164,9 @@ retry_irq_register:
>> printk(KERN_DEBUG "megasas: Failed to "
>> "register IRQ for vector %d.\n", i);
>> for (j = 0; j < i; j++) {
>> - irq_set_affinity_hint(
>> - instance->msixentry[j].vector,
>> NULL);
>> + if (smp_affinity_enable)
>> + irq_set_affinity_hint(
>> +
>> instance->msixentry[j].vector, NULL);
>> free_irq(
>> instance->msixentry[j].vector,
>> &instance->irq_context[j]);
>> @@ -5170,11 +5175,14 @@ retry_irq_register:
>> instance->msix_vectors = 0;
>> goto retry_irq_register;
>> }
>> - if
>> (irq_set_affinity_hint(instance->msixentry[i].vector,
>> - get_cpu_mask(cpu)))
>> - dev_err(&instance->pdev->dev, "Error setting"
>> - "affinity hint for cpu %d\n", cpu);
>> - cpu = cpumask_next(cpu, cpu_online_mask);
>> + if (smp_affinity_enable) {
>> + if
>> (irq_set_affinity_hint(instance->msixentry[i].vector,
>> + get_cpu_mask(cpu)))
>> + dev_err(&instance->pdev->dev,
>> + "Error setting affinity hint "
>> + "for cpu %d\n", cpu);
>> + cpu = cpumask_next(cpu, cpu_online_mask);
>> + }
>> }
>> } else {
>> instance->irq_context[0].instance = instance;
>> @@ -5233,8 +5241,9 @@ retry_irq_register:
>> instance->instancet->disable_intr(instance);
>> if (instance->msix_vectors)
>> for (i = 0; i < instance->msix_vectors; i++) {
>> - irq_set_affinity_hint(
>> - instance->msixentry[i].vector, NULL);
>> + if (smp_affinity_enable)
>> + irq_set_affinity_hint(
>> + instance->msixentry[i].vector, NULL);
>> free_irq(instance->msixentry[i].vector,
>>&instance->irq_context[i]);
>> }
>> @@ -5397,8 +5406,9 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t
>> state)
>>
>> if (instance->msix_vectors)
>> for (i = 0; i < instance->msix_vectors; i++) {
>> - irq_set_affinity_hint(
>> - instance->msixentry[i].vector, NULL);
>> + if (smp_affinity_enable)
>> + irq_set_affinity_hint(
>> + instance->msixentry[i].vector, NULL);
>> free_irq(instance->msixentry[i].vector,
>>&instance->irq_context[i]);
>> }
>> @@ -5507,8 +5517,9 @@ megasas_resume(st
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
[ +cc Peter Zijlstra, Ingo Molnar ]
On 09/10/2014 07:46 AM, Hans de Goede wrote:
> From: Sanjeev Sharma
>
> On some architecture spin_is_locked() always return false in
> uniprocessor configuration and therefore it would be advise to replace
> with lockdep_assert_held().
>
> Signed-off-by: Sanjeev Sharma
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/storage/uas.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..05b2d8e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info
> *devinfo,
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>
> uas_log_cmd_state(cmnd, caller);
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
This change isn't equivalent.
lockdep_assert_held() will continue to emit warnings; ie., there is no
"once" functionality. Same for the other changes below.
Regards,
Peter Hurley
> WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
> cmdinfo->state |= COMMAND_ABORTED;
> cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
> struct uas_dev_info *devinfo = cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> cmdinfo->state |= IS_IN_WORK_LIST;
> schedule_work(&devinfo->work);
> }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
> char *caller)
> struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & (COMMAND_INFLIGHT |
> DATA_IN_URB_INFLIGHT |
> DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
> struct urb *urb;
> int err;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & SUBMIT_STATUS_URB) {
> urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
> if (!urb)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/21] uas: zap_pending: data urbs should have completed at this time
On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: > uas_log_cmd_state(cmnd, __func__); > - /* all urbs are killed, clear inflight bits */ > - cmdinfo->state &= ~(COMMAND_INFLIGHT | > - DATA_IN_URB_INFLIGHT | > - DATA_OUT_URB_INFLIGHT); > + /* Sense urbs were killed, clear COMMAND_INFLIGHT > manually */ > + cmdinfo->state &= ~COMMAND_INFLIGHT; > cmnd->result = result << 16; > - uas_try_complete(cmnd, __func__); > + WARN_ON(uas_try_complete(cmnd, __func__) != 0); That looks like very bad style. WARN_ON() shouldn't have side effects. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 20/21] uas: Remove support for old sense ui as used in pre-production hardware
Hi, On 09/10/2014 04:06 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: >> I've access to a number of different uas devices now, and none of them use >> old style sense urbs. The only case where these code-paths trigger is with >> the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes >> status iu-s when it does not have any sense data, but uses new style >> sense iu-s regardless, as can be seen for scsi cmnds where there is sense >> data. > > This looks like a bad idea over all. Removing this actually makes the ASM1051 bridge, which is the only one ever sending 8 byte sense iu-s work better, as when it actually has sense data it sends > 16 byte sense iu-s with the sense data length at the location defined in the new 16 byte sense iu struct, and with the old code we then ignore the length field (as we use the wrong field) and instead use urb->actual_length - 8 bytes of the data as sense data, while in reality there only is urb->actual_length - 16. So either we drop support for the old style sense iu, at which point sense iu-s just works with the ASM1051, or we need to add some extra magic. > The version is in the spec. Not sure what you're trying to say here, the old sense-iu was only ever present in draft versions. The official uas-r04.pdf and the uas2r00.pdf draft both only define the new style 16 byte sense iu. > And you might see true USB<->SCSI bridges passing through the > sense data. Right, and we support that, this just drops support for the old style 8 byte sense-iu which was only present in the r00 draft of the spec. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] megaraid_sas : Add module parameter to disable IRQ-CPU affinity hint
On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
> For certain deployment, we may need to disable irq cpu affinity hint.
> This module parameter provides option for use to disable irq cpu affinity hint
> and allow irqbalancer to handle the rest.
Only curious , in which environments causes this which issues?
Thanks,
Tomas
>
> Signed-off-by: Sumit Saxena
> Signed-off-by: Kashyap Desai
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 60
> +++
> 1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 07255c1..086beee 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -89,6 +89,10 @@ module_param(resetwaittime, int, S_IRUGO);
> MODULE_PARM_DESC(resetwaittime, "Wait time in seconds after I/O timeout "
>"before resetting adapter. Default: 180");
>
> +int smp_affinity_enable = 1;
> +module_param(smp_affinity_enable, int, S_IRUGO);
> +MODULE_PARM_DESC(smp_affinity_enable, "SMP affinity feature enable/disbale
> Default: enable(1)");
> +
> MODULE_LICENSE("GPL");
> MODULE_VERSION(MEGASAS_VERSION);
> MODULE_AUTHOR("megaraidli...@lsi.com");
> @@ -5160,8 +5164,9 @@ retry_irq_register:
> printk(KERN_DEBUG "megasas: Failed to "
> "register IRQ for vector %d.\n", i);
> for (j = 0; j < i; j++) {
> - irq_set_affinity_hint(
> - instance->msixentry[j].vector,
> NULL);
> + if (smp_affinity_enable)
> + irq_set_affinity_hint(
> +
> instance->msixentry[j].vector, NULL);
> free_irq(
> instance->msixentry[j].vector,
> &instance->irq_context[j]);
> @@ -5170,11 +5175,14 @@ retry_irq_register:
> instance->msix_vectors = 0;
> goto retry_irq_register;
> }
> - if (irq_set_affinity_hint(instance->msixentry[i].vector,
> - get_cpu_mask(cpu)))
> - dev_err(&instance->pdev->dev, "Error setting"
> - "affinity hint for cpu %d\n", cpu);
> - cpu = cpumask_next(cpu, cpu_online_mask);
> + if (smp_affinity_enable) {
> + if
> (irq_set_affinity_hint(instance->msixentry[i].vector,
> + get_cpu_mask(cpu)))
> + dev_err(&instance->pdev->dev,
> + "Error setting affinity hint "
> + "for cpu %d\n", cpu);
> + cpu = cpumask_next(cpu, cpu_online_mask);
> + }
> }
> } else {
> instance->irq_context[0].instance = instance;
> @@ -5233,8 +5241,9 @@ retry_irq_register:
> instance->instancet->disable_intr(instance);
> if (instance->msix_vectors)
> for (i = 0; i < instance->msix_vectors; i++) {
> - irq_set_affinity_hint(
> - instance->msixentry[i].vector, NULL);
> + if (smp_affinity_enable)
> + irq_set_affinity_hint(
> + instance->msixentry[i].vector, NULL);
> free_irq(instance->msixentry[i].vector,
>&instance->irq_context[i]);
> }
> @@ -5397,8 +5406,9 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t
> state)
>
> if (instance->msix_vectors)
> for (i = 0; i < instance->msix_vectors; i++) {
> - irq_set_affinity_hint(
> - instance->msixentry[i].vector, NULL);
> + if (smp_affinity_enable)
> + irq_set_affinity_hint(
> + instance->msixentry[i].vector, NULL);
> free_irq(instance->msixentry[i].vector,
>&instance->irq_context[i]);
> }
> @@ -5507,8 +5517,9 @@ megasas_resume(struct pci_dev *pdev)
> printk(KERN_DEBUG "megasas: Failed to "
> "register IRQ for vector %d.\n", i);
> for (j = 0; j < i; j++) {
> - irq_set_affinity_hint(
> - instance->msixentry[j].vector,
> NULL);
> +
Re: [PATCH 10/21] uas: zap_pending: data urbs should have completed at this time
Hi, On 09/10/2014 04:10 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: >> uas_log_cmd_state(cmnd, __func__); >> - /* all urbs are killed, clear inflight bits */ >> - cmdinfo->state &= ~(COMMAND_INFLIGHT | >> - DATA_IN_URB_INFLIGHT | >> - DATA_OUT_URB_INFLIGHT); >> + /* Sense urbs were killed, clear COMMAND_INFLIGHT >> manually */ >> + cmdinfo->state &= ~COMMAND_INFLIGHT; >> cmnd->result = result << 16; >> - uas_try_complete(cmnd, __func__); >> + WARN_ON(uas_try_complete(cmnd, __func__) != 0); > > That looks like very bad style. WARN_ON() shouldn't > have side effects. A valid point, I'll respin this patch to store the result and check it separately. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 20/21] uas: Remove support for old sense ui as used in pre-production hardware
On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: > I've access to a number of different uas devices now, and none of them use > old style sense urbs. The only case where these code-paths trigger is with > the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes > status iu-s when it does not have any sense data, but uses new style > sense iu-s regardless, as can be seen for scsi cmnds where there is sense > data. This looks like a bad idea over all. The version is in the spec. And you might see true USB<->SCSI bridges passing through the sense data. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/21] uas: Do not log urb status error on cancellation
Hi, On 09/10/2014 04:00 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: >> Check for both type of cancellation codes for sense and data urbs. > > Then you should also check for -ESHUTDOWN (unplug of HC) The intent here is to stop log pollution when cancelling because of an abort / bus-reset. When people unplug a device with io pending they deserve all the log pollution they get IMHO. Also most of the time things seem to end with -EPROTO then. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/21] uas: Do not log urb status error on cancellation
On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: > Check for both type of cancellation codes for sense and data urbs. Then you should also check for -ESHUTDOWN (unplug of HC) Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 10/16] scsi: ufs: add UFS power management support
2014-09-10 20:54 GMT+09:00 Dolev Raviv :
> +static inline void ufshcd_enable_irq(struct ufs_hba *hba)
> +{
> + if (!hba->is_irq_enabled) {
> + enable_irq(hba->irq);
> + hba->is_irq_enabled = true;
> + }
> +}
> +
> +static inline void ufshcd_disable_irq(struct ufs_hba *hba)
> +{
> + if (hba->is_irq_enabled) {
> + disable_irq(hba->irq);
> + hba->is_irq_enabled = false;
> + }
> +}
This IRQ could be shared among several devices because it is requested
with IRQF_SHARED. So enable_irq()/disable_irq() should be replaced with
request_irq()/free_irq()? Otherwise other devices which share the same
IRQ will be malfunction while disabling IRQ.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] megaraid_sas : Extended VD support
On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
> Current MegaRAID firmware and hence the driver only supported 64VDs.
> E.g: If the user wants to create more than 64VD on a controller,
> it is not possible on current firmware/driver.
>
> New feature and requirement to support upto 256VD, firmware/driver/apps need
> changes.
> In addition to that there must be a backward compatibility of the new driver
> with the
> older firmware and vice versa.
>
> RAID map is the interface between Driver and FW to fetch all required
> fields(attributes) for each Virtual Drives.
> In the earlier design driver was using the FW copy of RAID map where as
> in the new design the Driver will keep the RAID map copy of its own; on which
> it will operate for any raid map access in fast path.
>
> Local driver raid map copy will provide ease of access through out the code
> and provide generic interface for future FW raid map changes.
>
> For the backward compatibility driver will notify FW that it supports 256VD
> to the FW in driver capability field.
> Based on the controller properly returned by the FW, the Driver will know
> whether it supports 256VD or not and will copy the RAID map accordingly.
>
> At any given time, driver will always have old or new Raid map.
> So with this changes, driver can also work in host lock less mode. Please
> see next patch which enable host lock less mode for megaraid_sas driver.
>
> Signed-off-by: Sumit Saxena
> Signed-off-by: Kashyap Desai
> ---
> drivers/scsi/megaraid/megaraid_sas.h| 73 +++---
> drivers/scsi/megaraid/megaraid_sas_base.c | 205
>
> drivers/scsi/megaraid/megaraid_sas_fp.c | 195 ++
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 118
> drivers/scsi/megaraid/megaraid_sas_fusion.h | 95 -
> 5 files changed, 502 insertions(+), 184 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> b/drivers/scsi/megaraid/megaraid_sas.h
> index e0f03e2..5dedf09 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -390,7 +390,6 @@ enum MR_LD_QUERY_TYPE {
> #define MR_EVT_FOREIGN_CFG_IMPORTED 0x00db
> #define MR_EVT_LD_OFFLINE 0x00fc
> #define MR_EVT_CTRL_HOST_BUS_SCAN_REQUESTED 0x0152
> -#define MAX_LOGICAL_DRIVES 64
>
> enum MR_PD_STATE {
> MR_PD_STATE_UNCONFIGURED_GOOD = 0x00,
> @@ -468,14 +467,14 @@ struct MR_LD_LIST {
> u8 state;
> u8 reserved[3];
> u64 size;
> - } ldList[MAX_LOGICAL_DRIVES];
> + } ldList[MAX_LOGICAL_DRIVES_EXT];
> } __packed;
>
> struct MR_LD_TARGETID_LIST {
> u32 size;
> u32 count;
> u8 pad[3];
> - u8 targetId[MAX_LOGICAL_DRIVES];
> + u8 targetId[MAX_LOGICAL_DRIVES_EXT];
> };
>
>
> @@ -941,6 +940,15 @@ struct megasas_ctrl_info {
> * HA cluster information
> */
> struct {
> +#if defined(__BIG_ENDIAN_BITFIELD)
> + u32 reserved:26;
> + u32 premiumFeatureMismatch:1;
> + u32 ctrlPropIncompatible:1;
> + u32 fwVersionMismatch:1;
> + u32 hwIncompatible:1;
> + u32 peerIsIncompatible:1;
> + u32 peerIsPresent:1;
> +#else
> u32 peerIsPresent:1;
> u32 peerIsIncompatible:1;
> u32 hwIncompatible:1;
> @@ -948,6 +956,7 @@ struct megasas_ctrl_info {
> u32 ctrlPropIncompatible:1;
> u32 premiumFeatureMismatch:1;
> u32 reserved:26;
> +#endif
> } cluster;
>
> char clusterId[16]; /*7D4h */
> @@ -962,9 +971,17 @@ struct megasas_ctrl_info {
> #if defined(__BIG_ENDIAN_BITFIELD)
> u32 reserved:25;
> u32 supportCrashDump:1;
> - u32 reserved1:6;
> + u32 supportMaxExtLDs:1;
> + u32 supportT10RebuildAssist:1;
> + u32 supportDisableImmediateIO:1;
> + u32 supportThermalPollInterval:1;
> + u32 supportPersonalityChange:2;
> #else
> - u32 reserved1:6;
> + u32 supportPersonalityChange:2;
> + u32 supportThermalPollInterval:1;
> + u32 supportDisableImmediateIO:1;
> + u32 supportT10RebuildAssist:1;
> + u32 supportMaxExtLDs:1;
> u32 supportCrashDump:1;
> u32 reserved:25;
> #endif
> @@ -979,13 +996,12 @@ struct megasas_ctrl_info {
> * ===
> */
> #define MEGASAS_MAX_PD_CHANNELS 2
> -#define MEGASAS_MAX_LD_CHANNELS 1
> +#define MEGASAS_MAX_LD_CHANNELS 2
> #define MEGAS
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
On Wed, Sep 10, 2014 at 03:15:41PM +0200, Hans de Goede wrote: > Hi, > > On 09/10/2014 02:54 PM, Oliver Neukum wrote: > > On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 09/10/2014 01:56 PM, Oliver Neukum wrote: > >>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: > Hi, > > Note this series is NOT intended for stable, but I accidentally > had "cc = sta...@vger.kernel.org" in my .git/config when sending > this series, please ignore for stable. > > NACK for stable. > >>> > >>> If this is not for stable, what do you intend to do about > >>> the problems in stable? For example patch#01 of this series > >>> looks like clear stable material to me. > >> > >> The plan for stable is mostly, as lame as that is, to make sure > >> we get all the right quirks in place so that error handling > >> does not get triggered, for now. > > > > How? A medium can be defect. Short of entirely disabling it, > > error handling will be triggered. > > I agree that this is a concern, but defective disks are not the > norm. All the bugs I've received sofar seem to be about incompatibilities > between the Linux uas/scsi stack and the device, not defective mediums. > > >> I agree that once this set has seen wider testing, we should > >> reconsider, and probably add it, to stable. But at this point > >> in time I'm worried that it may cause regressions, and as such > >> it is not stable material atm IHMO. > > > > Well, we would exchange something known to work imperfectly > > for something feared to work imperfectly. > > True. Note as said I'm not against this going into stable, I just don't > want to rush it into stable. So first lets get it reviewed and into > 3.18 (and see how it works for the users who have been having troubles > sofar, see my request for testing), and then see from there. > > I assume that you agree that this is (way) too late for 3.17? Yes it is. And I agree, let's test this out first, and if it solves problems, _then_ we can backport it to stable as needed. thanks for the patches, I'll queue them up for 3.18. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/21] uas: Fix resetting flag handling
Hi, First of all, thank you for reviewing this. On 09/10/2014 03:40 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: >> - Make sure we always hold the lock when setting / checking resetting >> - Check resetting before checking urb->status >> - Add missing check for resetting to uas_data_cmplt >> - Add missing check for resetting to uas_do_work > > Why is the checking for stat and data inconsistent? With stat urbs there is no direct reference from the urb to the scsi cmnd, since when using usb-2 the sense urbs may arrive in a different order then the cmnds where issued. instead for sense urbs we look up the cmnd by tag, likewise there is no reference from the cmnd back to the sense-urb. For data urbs however the urb->context pointer points directly to the cmnd struct, and the cmnd struct has a pointer back in the form of devinfo->data_[in|out]_urb. When the urb completes we want to NULL the devinfo->data_[in|out]_urb pointers even when resetting. So that we don't try to deref a pointer to a free-ed urb later on (this does not become an issue until after the "uas: Free data urbs on completion" commit). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/21] uas: Fix resetting flag handling
On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: > - Make sure we always hold the lock when setting / checking resetting > - Check resetting before checking urb->status > - Add missing check for resetting to uas_data_cmplt > - Add missing check for resetting to uas_do_work Why is the checking for stat and data inconsistent? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/21] uas: Remove task-management / abort error handling code
On Wed, 2014-09-10 at 13:46 +0200, Hans de Goede wrote: > This commit removes the abort / lun-reset error handling paths, and also the > taks-mgmt code since those are the only 2 task-mgmt users. Leaving only the > (tested and testable) usb-device-reset error handling path in place. > > Note I realize that this is somewhat of a big hammer, but currently people > are seeing very hard to debug oopses with uas. First let focus on making uas > work reliable, then we can later look into adding more fine grained error > handling. This hurts. But I suppose it is the conservative choice. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Wed, 2014-09-10 at 12:07 +0200, Ceriel Jacobs wrote: > Tom Gundersen schreef op 10-09-14 om 08:46: > >> >Indeed. What I proposed with a multiplier for the timeout for the > >> >different types of built in commands was deemed complex but saw no > >> >alternatives proposed despite my interest to work on one and > >> >clarifications noted that this was a design regression. Not quite sure > >> >what else I could have done here. I'm interested in learning what the > >> >better approach is for the future as if we want to marry init + kernel > >> >we need a smooth way for us to discuss design without getting worked > >> >up about it, or taking it personal. I really want this to work as I > >> >personally like systemd so far. > > How about this: keep the timeout global, but also introduce a > > (relatively short, say 10 or 15 seconds) timeout after which a warning > > is printed. Even if nothing is actually killed, having workers (be it > > insmod or something else) take longer than a couple of seconds is > > likely a sign that something is seriously off somewhere. > I don't agree with the statement that something is seriously off when it > takes more then 10 to 15 seconds. > > When probing only one hard disk drive, then I do agree that something is > seriously off after 10 to 15 seconds. Really? We keep explaining that arbitrary times are wrong. A while ago the Adaptec driver used to use 15s as its bus settle time after the initial reset (it's now a Kconfig variable set at 5s) and a Parallel bus takes a minimum of 4s to scan and has to be done sequentially. If any probed device is having difficulty, that can escalate way beyond this into the tens to hundreds of seconds. If your root disk is on it, you're waiting or not booting. > When probing a SAS bus with one hundred hard disk drives in standby > mode, then I do expect that to take longer then 10 to 15 seconds. Good luck with that even on SAS if you have a lot of expanders. For an installed system, you know what you need (usually root and possibly one other disc like /home), so you spawn all the insertions asynchronously and then wait for just the devices you need them but, since the alternative is panic when init isn't found, this wait better be quite long (if not forever, given the consequence is guaranteed failure). Everything else can be async, but, as I've pointed out before, it can be async in user space (fire and forget) instead of the kernel. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
Hi, On 09/10/2014 02:54 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote: >> Hi, >> >> On 09/10/2014 01:56 PM, Oliver Neukum wrote: >>> On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: Hi, Note this series is NOT intended for stable, but I accidentally had "cc = sta...@vger.kernel.org" in my .git/config when sending this series, please ignore for stable. NACK for stable. >>> >>> If this is not for stable, what do you intend to do about >>> the problems in stable? For example patch#01 of this series >>> looks like clear stable material to me. >> >> The plan for stable is mostly, as lame as that is, to make sure >> we get all the right quirks in place so that error handling >> does not get triggered, for now. > > How? A medium can be defect. Short of entirely disabling it, > error handling will be triggered. I agree that this is a concern, but defective disks are not the norm. All the bugs I've received sofar seem to be about incompatibilities between the Linux uas/scsi stack and the device, not defective mediums. >> I agree that once this set has seen wider testing, we should >> reconsider, and probably add it, to stable. But at this point >> in time I'm worried that it may cause regressions, and as such >> it is not stable material atm IHMO. > > Well, we would exchange something known to work imperfectly > for something feared to work imperfectly. True. Note as said I'm not against this going into stable, I just don't want to rush it into stable. So first lets get it reviewed and into 3.18 (and see how it works for the users who have been having troubles sofar, see my request for testing), and then see from there. I assume that you agree that this is (way) too late for 3.17? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
On Wed, 2014-09-10 at 14:00 +0200, Hans de Goede wrote: > Hi, > > On 09/10/2014 01:56 PM, Oliver Neukum wrote: > > On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: > >> Hi, > >> > >> Note this series is NOT intended for stable, but I accidentally > >> had "cc = sta...@vger.kernel.org" in my .git/config when sending > >> this series, please ignore for stable. > >> > >> NACK for stable. > > > > If this is not for stable, what do you intend to do about > > the problems in stable? For example patch#01 of this series > > looks like clear stable material to me. > > The plan for stable is mostly, as lame as that is, to make sure > we get all the right quirks in place so that error handling > does not get triggered, for now. How? A medium can be defect. Short of entirely disabling it, error handling will be triggered. > I agree that once this set has seen wider testing, we should > reconsider, and probably add it, to stable. But at this point > in time I'm worried that it may cause regressions, and as such > it is not stable material atm IHMO. Well, we would exchange something known to work imperfectly for something feared to work imperfectly. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Updated linux uas driver, please test
Hi All, I'm mailing all of you because you've reported various problems with the new uas support in kernel 3.16 and later. I've been working on making the uas driver more resilient to errors, as well as improved logging so we can easier figure out the cause of errors. I would like to ask you all to test a standalone version of the new uas driver with the devices you've been having trouble with before, and report the results to me. Testing instructions: 1) Remove any usb-storage.quirks= setting from the kernel commandline, and remove any /etc/modprobe.conf* files doing the same, boot your machine without the uas device attached. 2) Make sure your machine is set up for building kernel modules, usually this means installing kernel-devel and gcc packages, see your distributions documentation for more info 3) Download all files from here: https://fedorapeople.org/~jwrdegoede/uas/ And put them all in a single directory, named e.g. uas 4) Start a terminal, cd into the uas directory 5) Run the following commands: make sudo rmmod uas sudo insmod ./uas.ko 6) Connect your uas device 7) Wait for the disk to show up (wait circa 1 minute max), then do: dmesg > dmesg.log lsusb -v > lsusb.log 8) Test the uas disk Once done please send me a mail, in this mail please 1) Describe how the disk worked, did it show up in a reasonable time, and did it work? 2) Attach dmesg.log and lsusb.log Thanks & Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 04/11] megaraid_sas : Firmware crash dump feature support
>-Original Message-
>From: Tomas Henzl [mailto:the...@redhat.com]
>Sent: Tuesday, September 09, 2014 9:24 PM
>To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
>Cc: martin.peter...@oracle.com; h...@infradead.org;
>jbottom...@parallels.com; kashyap.de...@avagotech.com;
>aradf...@gmail.com
>Subject: Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature
>support
>
>On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>> This feature will provide similar interface as kernel crash dump feature.
>> When megaraid firmware encounter any crash, driver will collect the
>> firmware raw image and dump it into pre-configured location.
>>
>> Driver will allocate two different segment of memory.
>> #1 Non-DMA able large buffer (will be allocated on demand) to capture
>actual FW crash dump.
>> #2 DMA buffer (persistence allocation) just to do a arbitrator job.
>>
>> Firmware will keep writing Crash dump data in chucks of DMA buffer
>> size into #2, which will be copy back by driver to the host memory as
>described in #1.
>>
>> Driver-Firmware interface:
>> ==
>> A.) Host driver can allocate maximum 512MB Host memory to store crash
>dump data.
>>
>> This memory will be internal to the host and will not be exposed to the
>Firmware.
>> Driver may not be able to allocate 512 MB. In that case, driver will
>> do possible memory (available at run time) allocation to store crash dump
>data.
>>
>> Let’s call this buffer as Host Crash Buffer.
>>
>> Host Crash buffer will not be contigious as a whole, but it will have
>> multiple
>chunk of contigious memory.
>> This will be internal to driver and firmware/application are unaware of
>> it.
>> Partial allocation of Host Crash buffer may have valid information to
>> debug depending upon what was collected in that buffer and depending on
>nature of failure.
>>
>> Complete Crash dump is the best case, but we do want to capture partial
>buffer just to grab something rather than nothing.
>> Host Crash buffer will be allocated only when FW Crash dump data is
>> available, and will be deallocated once application copy Host Crash
>> buffer to
>the file.
>> Host Crash buffer size can be anything between 1MB to 512MB. (It will
>> be multiple of 1MBs)
>>
>>
>> B.) Irrespective of underlying Firmware capability of crash dump
>> support, driver will allocate DMA buffer at start of the day for each MR
>controllers.
>> Let’s call this buffer as “DMA Crash Buffer”.
>>
>> For this feature, size of DMA crash buffer will be 1MB.
>> (We will not gain much even if DMA buffer size is increased.)
>>
>> C.) Driver will now read Controller Info sending existing dcmd
>“MR_DCMD_CTRL_GET_INFO”.
>> Driver should extract the information from ctrl info provided by
>> firmware and figure out if firmware support crash dump feature or not.
>>
>> Driver will enable crash dump feature only if “Firmware support Crash
>> dump” + “Driver was able to create DMA Crash Buffer”.
>>
>> If either one from above is not set, Crash dump feature should be disable
>> in
>driver.
>> Firmware will enable crash dump feature only if “Driver Send DCMD-
>MR_DCMD_SET_CRASH_BUF_PARA with MR_CRASH_BUF_TURN_ON”
>>
>> Helper application/script should use sysfs parameter fw_crash_xxx to
>> actually copy data from host memory to the filesystem.
>
>Is it possible to store the crash dump data on filesystem on the same
>controller after the controller has crashed or do you expect that a use of
>another disk/controller?
The location where crash dump is to collected is configurable via config
file of application collecting dump.
Default config file settings is crash data will be collected on disk, on
which OS is booted up.
If Online controller reset(OCR) is enabled in crashed controller and OS disk
is behind same controller, crash data
Will be collected on same controller's filesystem.
There is one case, where crashed controller has OCR disabled and OS is
behind the crashed controller, crash data needs to collected behind
some other storage, then location of crash data collection needs to be
configured to some other disk.
Reason for the same is: crashed controller will be functional only after
system reboot and we need to collect data before system reboot,
so crash data needs to be collected behind disk, which is not on crashed
controller.
>With several controllers in a system this may take a lot memory, could you
>also
>in case when a kdump kernel is running lower it, by not using this feature?
>
Agreed, we will disable this feature for kdump kernel by adding
"reset_devices" global varaiable.
That check is required for only one place, throughout the code, this feature
will remain disabled.
Code snippet for the same-
instance->crash_dump_drv_support = (!reset_devices) &&
crashdump_enable &&
instance->crash_dump_fw_support &&
instance->crash_dump_buf);
if(instance->crash_dump_drv_support) {
printk(KERN_INFO "me
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
Hi, On 09/10/2014 01:56 PM, Oliver Neukum wrote: > On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: >> Hi, >> >> Note this series is NOT intended for stable, but I accidentally >> had "cc = sta...@vger.kernel.org" in my .git/config when sending >> this series, please ignore for stable. >> >> NACK for stable. > > If this is not for stable, what do you intend to do about > the problems in stable? For example patch#01 of this series > looks like clear stable material to me. The plan for stable is mostly, as lame as that is, to make sure we get all the right quirks in place so that error handling does not get triggered, for now. I agree that once this set has seen wider testing, we should reconsider, and probably add it, to stable. But at this point in time I'm worried that it may cause regressions, and as such it is not stable material atm IHMO. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
-Original Message-
From: Oliver Neukum [mailto:oneu...@suse.de]
Sent: Wednesday, September 10, 2014 5:26 PM
To: Hans de Goede
Cc: Greg Kroah-Hartman; linux-...@vger.kernel.org; linux-scsi@vger.kernel.org;
sta...@vger.kernel.org; Sharma, Sanjeev
Subject: Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with
lockdep_assert_held()
On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote:
> Hi,
>
> Note this series is NOT intended for stable, but I accidentally had
> "cc = sta...@vger.kernel.org" in my .git/config when sending this
> series, please ignore for stable.
>
> NACK for stable.
If this is not for stable, what do you intend to do about the problems in
stable? For example patch#01 of this series looks like clear stable material to
me.
Regards
Oliver
This is not clear to me also. Please Clarify "Note this series is NOT intended
for stable" means ?
Regards
Sanjeev Sharma
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
On Wed, 2014-09-10 at 13:48 +0200, Hans de Goede wrote: > Hi, > > Note this series is NOT intended for stable, but I accidentally > had "cc = sta...@vger.kernel.org" in my .git/config when sending > this series, please ignore for stable. > > NACK for stable. If this is not for stable, what do you intend to do about the problems in stable? For example patch#01 of this series looks like clear stable material to me. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 12/16] scsi: ufs: Add support for clock gating
From: Sahitya Tummala
The UFS controller clocks can be gated after certain period of
inactivity, which is typically less than runtime suspend timeout.
In addition to clocks the link will also be put into Hibern8 mode
to save more power.
The clock gating can be turned on by enabling the capability
UFSHCD_CAP_CLK_GATING. To enable entering into Hibern8 mode as part of
clock gating, set the capability UFSHCD_CAP_HIBERN8_WITH_CLK_GATING.
The tracing events for clock gating can be enabled through debugfs as:
echo 1 > /sys/kernel/debug/tracing/events/ufs/ufshcd_clk_gating/enable
cat /sys/kernel/debug/tracing/trace_pipe
Signed-off-by: Sahitya Tummala
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index bfa9a75..c762a6a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -177,6 +177,11 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
static void ufshcd_hba_exit(struct ufs_hba *hba);
static int ufshcd_probe_hba(struct ufs_hba *hba);
+static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
+bool skip_ref_clk);
+static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
+static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
+static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *desired_pwr_mode);
@@ -498,6 +503,231 @@ static inline int ufshcd_is_hba_active(struct ufs_hba
*hba)
return (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) & 0x1) ? 0 : 1;
}
+static void ufshcd_ungate_work(struct work_struct *work)
+{
+ int ret;
+ unsigned long flags;
+ struct ufs_hba *hba = container_of(work, struct ufs_hba,
+ clk_gating.ungate_work);
+
+ cancel_delayed_work_sync(&hba->clk_gating.gate_work);
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ if (hba->clk_gating.state == CLKS_ON) {
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ goto unblock_reqs;
+ }
+
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ ufshcd_setup_clocks(hba, true);
+
+ /* Exit from hibern8 */
+ if (ufshcd_can_hibern8_during_gating(hba)) {
+ /* Prevent gating in this path */
+ hba->clk_gating.is_suspended = true;
+ if (ufshcd_is_link_hibern8(hba)) {
+ ret = ufshcd_uic_hibern8_exit(hba);
+ if (ret)
+ dev_err(hba->dev, "%s: hibern8 exit failed
%d\n",
+ __func__, ret);
+ else
+ ufshcd_set_link_active(hba);
+ }
+ hba->clk_gating.is_suspended = false;
+ }
+unblock_reqs:
+ scsi_unblock_requests(hba->host);
+}
+
+/**
+ * ufshcd_hold - Enable clocks that were gated earlier due to ufshcd_release.
+ * Also, exit from hibern8 mode and set the link as active.
+ * @hba: per adapter instance
+ * @async: This indicates whether caller should ungate clocks asynchronously.
+ */
+int ufshcd_hold(struct ufs_hba *hba, bool async)
+{
+ int rc = 0;
+ unsigned long flags;
+
+ if (!ufshcd_is_clkgating_allowed(hba))
+ goto out;
+start:
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ hba->clk_gating.active_reqs++;
+
+ switch (hba->clk_gating.state) {
+ case CLKS_ON:
+ break;
+ case REQ_CLKS_OFF:
+ if (cancel_delayed_work(&hba->clk_gating.gate_work)) {
+ hba->clk_gating.state = CLKS_ON;
+ break;
+ }
+ /*
+* If we here, it means gating work is either done or
+* currently running. Hence, fall through to cancel gating
+* work and to enable clocks.
+*/
+ case CLKS_OFF:
+ scsi_block_requests(hba->host);
+ hba->clk_gating.state = REQ_CLKS_ON;
+ schedule_work(&hba->clk_gating.ungate_work);
+ /*
+* fall through to check if we should wait for this
+* work to be done or not.
+*/
+ case REQ_CLKS_ON:
+ if (async) {
+ rc = -EAGAIN;
+ hba->clk_gating.active_reqs--;
+ break;
+ } else {
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ flush_work(&hba->clk_gating.ungate_work);
+ /* Make sure state is CLKS_ON before returning */
+ goto start;
+ }
+ default:
+ dev_err(hba->dev, "%s: clk gating is in invalid
[PATCH V3 09/16] scsi: ufs: introduce well known logical unit in ufs
From: Subhash Jadavani
UFS device may have standard LUs and LUN id could be from 0x00 to 0x7F.
UFS device specification use "Peripheral Device Addressing Format"
(SCSI SAM-5) for standard LUs.
UFS device may also have the Well Known LUs (also referred as W-LU) which
again could be from 0x00 to 0x7F. For W-LUs, UFS device specification only
allows the "Extended Addressing Format" (SCSI SAM-5) which means the W-LUNs
would start from 0xC100 onwards.
This means max. LUN number reported from UFS device could be 0xC17F hence
this patch advertise the "max_lun" as 0xC17F which will allow SCSI mid
layer to detect the W-LUs as well.
But once the W-LUs are detected, UFSHCD driver may get the commands with
SCSI LUN id upto 0xC17F but UPIU LUN id field is only 8-bit wide so it
requires the mapping of SCSI LUN id to UPIU LUN id. This patch also add
support for this mapping.
Signed-off-by: Subhash Jadavani
Signed-off-by: Dolev Raviv
Signed-off-by: Sujit Reddy Thumma
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index b0e1f62..bcc3a7f 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -49,9 +49,28 @@
#define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
(byte1 << 8) | (byte0))
-
+/*
+ * UFS device may have standard LUs and LUN id could be from 0x00 to
+ * 0x7F. Standard LUs use "Peripheral Device Addressing Format".
+ * UFS device may also have the Well Known LUs (also referred as W-LU)
+ * which again could be from 0x00 to 0x7F. For W-LUs, device only use
+ * the "Extended Addressing Format" which means the W-LUNs would be
+ * from 0xc100 (SCSI_W_LUN_BASE) onwards.
+ * This means max. LUN number reported from UFS device could be 0xC17F.
+ */
+#define UFS_UPIU_MAX_UNIT_NUM_ID 0x7F
+#define UFS_MAX_LUNS (SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID)
+#define UFS_UPIU_WLUN_ID (1 << 7)
#define UFS_UPIU_MAX_GENERAL_LUN 8
+/* Well known logical unit id in LUN field of UPIU */
+enum {
+ UFS_UPIU_REPORT_LUNS_WLUN = 0x81,
+ UFS_UPIU_UFS_DEVICE_WLUN= 0xD0,
+ UFS_UPIU_BOOT_WLUN = 0xB0,
+ UFS_UPIU_RPMB_WLUN = 0xC4,
+};
+
/*
* UFS Protocol Information Unit related definitions
*/
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a851323..807a730 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -100,7 +100,6 @@ static u32 ufs_query_desc_max_size[] = {
enum {
UFSHCD_MAX_CHANNEL = 0,
UFSHCD_MAX_ID = 1,
- UFSHCD_MAX_LUNS = 8,
UFSHCD_CMD_PER_LUN = 32,
UFSHCD_CAN_QUEUE= 32,
};
@@ -902,6 +901,21 @@ static int ufshcd_compose_upiu(struct ufs_hba *hba, struct
ufshcd_lrb *lrbp)
}
/**
+ * ufshcd_scsi_to_upiu_lun - maps scsi LUN to UPIU LUN
+ * @scsi_lun: scsi LUN id
+ *
+ * Returns UPIU LUN id
+ */
+static inline u8 ufshcd_scsi_to_upiu_lun(unsigned int scsi_lun)
+{
+ if (scsi_is_wlun(scsi_lun))
+ return (scsi_lun & UFS_UPIU_MAX_UNIT_NUM_ID)
+ | UFS_UPIU_WLUN_ID;
+ else
+ return scsi_lun & UFS_UPIU_MAX_UNIT_NUM_ID;
+}
+
+/**
* ufshcd_queuecommand - main entry point for SCSI requests
* @cmd: command from SCSI Midlayer
* @done: call back function
@@ -959,7 +973,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
struct scsi_cmnd *cmd)
lrbp->sense_bufflen = SCSI_SENSE_BUFFERSIZE;
lrbp->sense_buffer = cmd->sense_buffer;
lrbp->task_tag = tag;
- lrbp->lun = cmd->device->lun;
+ lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
lrbp->intr_cmd = false;
lrbp->command_type = UTP_CMD_TYPE_SCSI;
@@ -1513,7 +1527,7 @@ static inline int ufshcd_read_unit_desc_param(struct
ufs_hba *hba,
* Unit descriptors are only available for general purpose LUs (LUN id
* from 0 to 7) and RPMB Well known LU.
*/
- if (lun >= UFS_UPIU_MAX_GENERAL_LUN)
+ if (lun != UFS_UPIU_RPMB_WLUN && (lun >= UFS_UPIU_MAX_GENERAL_LUN))
return -EOPNOTSUPP;
return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
@@ -2144,6 +2158,44 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
}
/**
+ * ufshcd_set_queue_depth - set lun queue depth
+ * @sdev: pointer to SCSI device
+ *
+ * Read bLUQueueDepth value and activate scsi tagged command
+ * queueing. For WLUN, queue depth is set to 1. For best-effort
+ * cases (bLUQueueDepth = 0) the queue depth is set to a maximum
+ * value that host can queue.
+ */
+static void ufshcd_set_queue_depth(struct scsi_device *sdev)
+{
+ int ret = 0;
+ u8 lun_qdepth;
+ struct ufs_hba *hba;
+
+ hba = shost_priv(sdev->host);
+
+ lun_qdepth = hba->nutrs;
+ ret = ufshcd_read_unit_desc_param(hba,
+ ufshcd_scsi_to_upiu_lun(sdev->lun
[PATCH V3 14/16] scsi: ufs: Add support for clock scaling using devfreq framework
From: Sahitya Tummala
The clocks for UFS device will be managed by generic DVFS (Dynamic
Voltage and Frequency Scaling) framework within kernel. This devfreq
framework works with different governors to scale the clocks. By default,
UFS devices uses simple_ondemand governor which scales the clocks up if
the load is more than upthreshold and scales down if the load is less than
downthreshold.
Signed-off-by: Sahitya Tummala
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index f07f901..6e07b2a 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -35,6 +35,8 @@
config SCSI_UFSHCD
tristate "Universal Flash Storage Controller Driver Core"
depends on SCSI && SCSI_DMA
+ select PM_DEVFREQ
+ select DEVFREQ_GOV_SIMPLE_ONDEMAND
---help---
This selects the support for UFS devices in Linux, say Y and make
sure that you know the name of your UFS host adapter (the card
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c762a6a..0bbb933 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -38,6 +38,7 @@
*/
#include
+#include
#include "ufshcd.h"
#include "unipro.h"
@@ -536,6 +537,8 @@ static void ufshcd_ungate_work(struct work_struct *work)
hba->clk_gating.is_suspended = false;
}
unblock_reqs:
+ if (ufshcd_is_clkscaling_enabled(hba))
+ devfreq_resume_device(hba->devfreq);
scsi_unblock_requests(hba->host);
}
@@ -552,10 +555,10 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
if (!ufshcd_is_clkgating_allowed(hba))
goto out;
-start:
spin_lock_irqsave(hba->host->host_lock, flags);
hba->clk_gating.active_reqs++;
+start:
switch (hba->clk_gating.state) {
case CLKS_ON:
break;
@@ -586,6 +589,7 @@ start:
spin_unlock_irqrestore(hba->host->host_lock, flags);
flush_work(&hba->clk_gating.ungate_work);
/* Make sure state is CLKS_ON before returning */
+ spin_lock_irqsave(hba->host->host_lock, flags);
goto start;
}
default:
@@ -627,6 +631,11 @@ static void ufshcd_gate_work(struct work_struct *work)
ufshcd_set_link_hibern8(hba);
}
+ if (ufshcd_is_clkscaling_enabled(hba)) {
+ devfreq_suspend_device(hba->devfreq);
+ hba->clk_scaling.window_start_t = 0;
+ }
+
if (!ufshcd_is_link_active(hba))
ufshcd_setup_clocks(hba, false);
else
@@ -728,6 +737,32 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
device_remove_file(hba->dev, &hba->clk_gating.delay_attr);
}
+/* Must be called with host lock acquired */
+static void ufshcd_clk_scaling_start_busy(struct ufs_hba *hba)
+{
+ if (!ufshcd_is_clkscaling_enabled(hba))
+ return;
+
+ if (!hba->clk_scaling.is_busy_started) {
+ hba->clk_scaling.busy_start_t = ktime_get();
+ hba->clk_scaling.is_busy_started = true;
+ }
+}
+
+static void ufshcd_clk_scaling_update_busy(struct ufs_hba *hba)
+{
+ struct ufs_clk_scaling *scaling = &hba->clk_scaling;
+
+ if (!ufshcd_is_clkscaling_enabled(hba))
+ return;
+
+ if (!hba->outstanding_reqs && scaling->is_busy_started) {
+ scaling->tot_busy_t += ktime_to_us(ktime_sub(ktime_get(),
+ scaling->busy_start_t));
+ scaling->busy_start_t = ktime_set(0, 0);
+ scaling->is_busy_started = false;
+ }
+}
/**
* ufshcd_send_command - Send SCSI or device management commands
* @hba: per adapter instance
@@ -736,6 +771,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
static inline
void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
{
+ ufshcd_clk_scaling_start_busy(hba);
__set_bit(task_tag, &hba->outstanding_reqs);
ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
}
@@ -2992,6 +3028,8 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
/* clear corresponding bits of completed commands */
hba->outstanding_reqs ^= completed_reqs;
+ ufshcd_clk_scaling_update_busy(hba);
+
/* we might have free'd some tags above */
wake_up(&hba->dev_cmd.tag_wq);
}
@@ -4024,6 +4062,10 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
if (!hba->is_init_prefetch)
hba->is_init_prefetch = true;
+ /* Resume devfreq after UFS device is detected */
+ if (ufshcd_is_clkscaling_enabled(hba))
+ devfreq_resume_device(hba->devfreq);
+
out:
/*
* If we failed to initialize the device or the device is not
@@ -4325,6 +4367,7 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
[PATCH V3 15/16] scsi: ufs: tune bkops while power managment events
From: Subhash Jadavani
Add capability to control the auto bkops during suspend.
If host explicitly enables the auto bkops (background operation) on device
then only device would perform the bkops on its own. If auto bkops is not
enabled explicitly and if the device reaches to state where it must do
background operation, device would raise the urgent bkops exception event
to host and then host will enable the auto bkops on device. This patch
adds the option to choose whether auto bkops should be enabled during
runtime suspend or not. Since we don't want to keep the device active to
perform the non critical bkops, host will enable urgent bkops only.
Keep auto-bkops enabled after resume if urgent bkops needed.
If device bkops status shows that its in critical need of executing
background operations, host should allow the device to continue doing
background operations.
Signed-off-by: Subhash Jadavani
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0bbb933..33984cc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4719,13 +4719,19 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum
ufs_pm_op pm_op)
}
if (ufshcd_is_runtime_pm(pm_op)) {
- /*
-* The device is idle with no requests in the queue,
-* allow background operations if needed.
-*/
- ret = ufshcd_bkops_ctrl(hba, BKOPS_STATUS_NON_CRITICAL);
- if (ret)
- goto enable_gating;
+ if (ufshcd_can_autobkops_during_suspend(hba)) {
+ /*
+* The device is idle with no requests in the queue,
+* allow background operations if bkops status shows
+* that performance might be impacted.
+*/
+ ret = ufshcd_urgent_bkops(hba);
+ if (ret)
+ goto enable_gating;
+ } else {
+ /* make sure that auto bkops is disabled */
+ ufshcd_disable_auto_bkops(hba);
+ }
}
if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
@@ -4871,7 +4877,11 @@ static int ufshcd_resume(struct ufs_hba *hba, enum
ufs_pm_op pm_op)
goto set_old_link_state;
}
- ufshcd_disable_auto_bkops(hba);
+ /*
+* If BKOPs operations are urgently needed at this moment then
+* keep auto-bkops enabled or else disable it.
+*/
+ ufshcd_urgent_bkops(hba);
hba->clk_gating.is_suspended = false;
if (ufshcd_is_clkscaling_enabled(hba))
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index d5699d0..b94b835 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -466,6 +466,8 @@ struct ufs_hba {
#define UFSHCD_CAP_HIBERN8_WITH_CLK_GATING (1 << 1)
/* Allow dynamic clk scaling */
#define UFSHCD_CAP_CLK_SCALING (1 << 2)
+ /* Allow auto bkops to enabled during runtime suspend */
+#define UFSHCD_CAP_AUTO_BKOPS_SUSPEND (1 << 3)
struct devfreq *devfreq;
struct ufs_clk_scaling clk_scaling;
@@ -484,6 +486,11 @@ static inline int ufshcd_is_clkscaling_enabled(struct
ufs_hba *hba)
{
return hba->caps & UFSHCD_CAP_CLK_SCALING;
}
+static inline bool ufshcd_can_autobkops_during_suspend(struct ufs_hba *hba)
+{
+ return hba->caps & UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
+}
+
#define ufshcd_writel(hba, val, reg) \
writel((val), (hba)->mmio_base + (reg))
#define ufshcd_readl(hba, reg) \
--
1.8.5.2
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 10/16] scsi: ufs: add UFS power management support
From: Subhash Jadavani
This patch adds support for UFS device and UniPro link power management
during runtime/system PM.
Main idea is to define multiple UFS low power levels based on UFS device
and UFS link power states. This would allow any specific platform or pci
driver to choose the best suited low power level during runtime and
system suspend based on their power goals.
bkops handlig:
To put the UFS device in sleep state when bkops is disabled, first query
the bkops status from the device and enable bkops on device only if
device needs time to perform the bkops.
START_STOP handling:
Before sending START_STOP_UNIT to the device well-known logical unit
(w-lun) to make sure that the device w-lun unit attention condition is
cleared.
Write protection:
UFS device specification allows LUs to be write protected, either
permanently or power on write protected. If any LU is power on write
protected and if the card is power cycled (by powering off VCCQ and/or
VCC rails), LU's write protect status would be lost. So this means those
LUs can be written now. To ensures that UFS device is power cycled only
if the power on protect is not set for any of the LUs, check if power on
write protect is set and if device is in sleep/power-off state & link in
inactive state (Hibern8 or OFF state).
If none of the Logical Units on UFS device is power on write protected
then all UFS device power rails (VCC, VCCQ & VCCQ2) can be turned off if
UFS device is in power-off state and UFS link is in OFF state. But current
implementation would disable all device power rails even if UFS link is
not in OFF state.
Low power mode:
If UFS link is in OFF state then UFS host controller can be power collapsed
to avoid leakage current from it. Note that if UFS host controller is power
collapsed, full UFS reinitialization will be required on resume to
re-establish the link between host and device.
Signed-off-by: Subhash Jadavani
Signed-off-by: Dolev Raviv
Signed-off-by: Sujit Reddy Thumma
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index bcc3a7f..2a82959 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -129,6 +129,7 @@ enum {
/* Flag idn for Query Requests*/
enum flag_idn {
QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
+ QUERY_FLAG_IDN_PWR_ON_WPE = 0x03,
QUERY_FLAG_IDN_BKOPS_EN = 0x04,
};
@@ -194,6 +195,18 @@ enum unit_desc_param {
UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22,
};
+/*
+ * Logical Unit Write Protect
+ * 00h: LU not write protected
+ * 01h: LU write protected when fPowerOnWPEn =1
+ * 02h: LU permanently write protected when fPermanentWPEn =1
+ */
+enum ufs_lu_wp_type {
+ UFS_LU_NO_WP= 0x00,
+ UFS_LU_POWER_ON_WP = 0x01,
+ UFS_LU_PERM_WP = 0x02,
+};
+
/* bActiveICCLevel parameter current units */
enum {
UFSHCD_NANO_AMP = 0,
@@ -226,11 +239,12 @@ enum {
};
/* Background operation status */
-enum {
+enum bkops_status {
BKOPS_STATUS_NO_OP = 0x0,
BKOPS_STATUS_NON_CRITICAL= 0x1,
BKOPS_STATUS_PERF_IMPACT = 0x2,
BKOPS_STATUS_CRITICAL= 0x3,
+ BKOPS_STATUS_MAX = BKOPS_STATUS_CRITICAL,
};
/* UTP QUERY Transaction Specific Fields OpCode */
@@ -291,6 +305,14 @@ enum {
UPIU_TASK_MANAGEMENT_FUNC_FAILED= 0x05,
UPIU_INCORRECT_LOGICAL_UNIT_NO = 0x09,
};
+
+/* UFS device power modes */
+enum ufs_dev_pwr_mode {
+ UFS_ACTIVE_PWR_MODE = 1,
+ UFS_SLEEP_PWR_MODE = 2,
+ UFS_POWERDOWN_PWR_MODE = 3,
+};
+
/**
* struct utp_upiu_header - UPIU header structure
* @dword_0: UPIU header DW-0
@@ -437,6 +459,12 @@ struct ufs_query_res {
#define UFS_VREG_VCCQ2_MIN_UV 165 /* uV */
#define UFS_VREG_VCCQ2_MAX_UV 195 /* uV */
+/*
+ * VCCQ & VCCQ2 current requirement when UFS device is in sleep state
+ * and link is in Hibern8 state.
+ */
+#define UFS_VREG_LPM_LOAD_UA 1000 /* uA */
+
struct ufs_vreg {
struct regulator *reg;
const char *name;
@@ -453,4 +481,10 @@ struct ufs_vreg_info {
struct ufs_vreg *vccq2;
};
+struct ufs_dev_info {
+ bool f_power_on_wp_en;
+ /* Keeps information if any of the LU is power on write protected */
+ bool is_lu_power_on_wp;
+};
+
#endif /* End of Header */
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 2a26faa..955ed55 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -43,34 +43,24 @@
* @pdev: pointer to PCI device handle
* @state: power state
*
- * Returns -ENOSYS
+ * Returns 0 if successful
+ * Returns non-zero otherwise
*/
static int ufshcd_pci_suspend(struct device *dev)
{
- /*
-* TODO:
-* 1. Call ufshcd_suspend
-* 2. Do bus specific power management
-*/
-
- return -ENOSYS;
+ return ufshcd_system_suspend(dev_get_drvdata(dev));
[PATCH V3 13/16] scsi: ufs: Add freq-table-hz property for UFS device
From: Sahitya Tummala
Add freq-table-hz propery for UFS device to keep track of
frequencies supported by UFS clocks.
Signed-off-by: Sahitya Tummala
Signed-off-by: Dolev Raviv
diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index b0f791a..e73a619 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -24,11 +24,11 @@ Optional properties:
- clocks: List of phandle and clock specifier pairs
- clock-names : List of clock input name strings sorted in the same
order as the clocks property.
-- max-clock-frequency-hz : List of maximum operating frequency stored in the
same
- order as the clocks property. If this property is
not
- defined or a value in the array is "0" then it is
assumed
- that the frequency is set by the parent clock or a
- fixed rate clock source.
+- freq-table-hz: Array of operating frequencies
stored in the same
+ order as the clocks property. If this property is not
+ defined or a value in the array is "0" then it is
assumed
+ that the frequency is set by the parent clock or a
+ fixed rate clock source.
Note: If above properties are not defined it can be assumed that the supply
regulators or clocks are always on.
@@ -49,5 +49,5 @@ Example:
clocks = <&core 0>, <&ref 0>, <&iface 0>;
clock-names = "core_clk", "ref_clk", "iface_clk";
- max-clock-frequency-hz = <1 1920 0>;
+ freq-table-hz = <1 2>, <0 0>, <0 0>;
};
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index edaccd0..551be95 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -63,6 +63,8 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
char *name;
u32 *clkfreq = NULL;
struct ufs_clk_info *clki;
+ int len = 0;
+ size_t sz = 0;
if (!np)
goto out;
@@ -82,39 +84,59 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (cnt <= 0)
goto out;
- clkfreq = kzalloc(cnt * sizeof(*clkfreq), GFP_KERNEL);
+ if (!of_get_property(np, "freq-table-hz", &len)) {
+ dev_info(dev, "freq-table-hz property not specified\n");
+ goto out;
+ }
+
+ if (len <= 0)
+ goto out;
+
+ sz = len / sizeof(*clkfreq);
+ if (sz != 2 * cnt) {
+ dev_err(dev, "%s len mismatch\n", "freq-table-hz");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ clkfreq = devm_kzalloc(dev, sz * sizeof(*clkfreq),
+ GFP_KERNEL);
if (!clkfreq) {
+ dev_err(dev, "%s: no memory\n", "freq-table-hz");
ret = -ENOMEM;
- dev_err(dev, "%s: memory alloc failed\n", __func__);
goto out;
}
- ret = of_property_read_u32_array(np,
- "max-clock-frequency-hz", clkfreq, cnt);
+ ret = of_property_read_u32_array(np, "freq-table-hz",
+ clkfreq, sz);
if (ret && (ret != -EINVAL)) {
- dev_err(dev, "%s: invalid max-clock-frequency-hz property,
%d\n",
- __func__, ret);
- goto out;
+ dev_err(dev, "%s: error reading array %d\n",
+ "freq-table-hz", ret);
+ goto free_clkfreq;
}
- for (i = 0; i < cnt; i++) {
+ for (i = 0; i < sz; i += 2) {
ret = of_property_read_string_index(np,
- "clock-names", i, (const char **)&name);
+ "clock-names", i/2, (const char **)&name);
if (ret)
- goto out;
+ goto free_clkfreq;
clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
if (!clki) {
ret = -ENOMEM;
- goto out;
+ goto free_clkfreq;
}
- clki->max_freq = clkfreq[i];
+ clki->min_freq = clkfreq[i];
+ clki->max_freq = clkfreq[i+1];
clki->name = kstrdup(name, GFP_KERNEL);
+ dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
+ clki->min_freq, clki->max_freq, clki->name);
list_add_tail(&clki->list, &hba->clk_list_head);
}
-out:
+free_clkfreq:
kfree(clkfreq);
+out:
return ret;
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs
[PATCH V3 04/16] scsi: ufs: Add regulator enable support
From: Sujit Reddy Thumma
UFS devices are powered by at most three external power supplies -
- VCC - The flash memory core power supply, 2.7V to 3.6V or 1.70V to 1.95V
- VCCQ - The controller and I/O power supply, 1.1V to 1.3V
- VCCQ2 - Secondary controller and/or I/O power supply, 1.65V to 1.95V
For some devices VCCQ or VCCQ2 are optional as they can be
generated using internal LDO inside the UFS device.
Add DT bindings for voltage regulators that can be controlled
from host driver.
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index 20468b2..65e3117 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -8,9 +8,33 @@ Required properties:
- interrupts:
- reg :
+Optional properties:
+- vcc-supply: phandle to VCC supply regulator node
+- vccq-supply : phandle to VCCQ supply regulator node
+- vccq2-supply : phandle to VCCQ2 supply regulator node
+- vcc-supply-1p8: For embedded UFS devices, valid VCC range is
1.7-1.95V
+ or 2.7-3.6V. This boolean property when set,
specifies
+ to use low voltage range of 1.7-1.95V. Note for
external
+ UFS cards this property is invalid and valid VCC
range is
+ always 2.7-3.6V.
+- vcc-max-microamp : specifies max. load that can be drawn from vcc supply
+- vccq-max-microamp : specifies max. load that can be drawn from vccq
supply
+- vccq2-max-microamp: specifies max. load that can be drawn from vccq2
supply
+
+Note: If above properties are not defined it can be assumed that the supply
+regulators are always on.
+
Example:
ufshc@0xfc598000 {
compatible = "jedec,ufs-1.1";
reg = <0xfc598000 0x800>;
interrupts = <0 28 0>;
+
+ vcc-supply = <&xxx_reg1>;
+ vcc-supply-1p8;
+ vccq-supply = <&xxx_reg2>;
+ vccq2-supply = <&xxx_reg3>;
+ vcc-max-microamp = 50;
+ vccq-max-microamp = 20;
+ vccq2-max-microamp = 20;
};
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index fafcf5e..729ce7d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -362,4 +362,29 @@ struct ufs_query_res {
struct utp_upiu_query upiu_res;
};
+#define UFS_VREG_VCC_MIN_UV 270 /* uV */
+#define UFS_VREG_VCC_MAX_UV 360 /* uV */
+#define UFS_VREG_VCC_1P8_MIN_UV170 /* uV */
+#define UFS_VREG_VCC_1P8_MAX_UV195 /* uV */
+#define UFS_VREG_VCCQ_MIN_UV 110 /* uV */
+#define UFS_VREG_VCCQ_MAX_UV 130 /* uV */
+#define UFS_VREG_VCCQ2_MIN_UV 165 /* uV */
+#define UFS_VREG_VCCQ2_MAX_UV 195 /* uV */
+
+struct ufs_vreg {
+ struct regulator *reg;
+ const char *name;
+ bool enabled;
+ int min_uV;
+ int max_uV;
+ int min_uA;
+ int max_uA;
+};
+
+struct ufs_vreg_info {
+ struct ufs_vreg *vcc;
+ struct ufs_vreg *vccq;
+ struct ufs_vreg *vccq2;
+};
+
#endif /* End of Header */
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index d727b1a..51e47c4 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -53,6 +53,99 @@ static struct ufs_hba_variant_ops *get_variant_ops(struct
device *dev)
return NULL;
}
+#define MAX_PROP_SIZE 32
+static int ufshcd_populate_vreg(struct device *dev, const char *name,
+ struct ufs_vreg **out_vreg)
+{
+ int ret = 0;
+ char prop_name[MAX_PROP_SIZE];
+ struct ufs_vreg *vreg = NULL;
+ struct device_node *np = dev->of_node;
+
+ if (!np) {
+ dev_err(dev, "%s: non DT initialization\n", __func__);
+ goto out;
+ }
+
+ snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", name);
+ if (!of_parse_phandle(np, prop_name, 0)) {
+ dev_info(dev, "%s: Unable to find %s regulator, assuming
enabled\n",
+ __func__, prop_name);
+ goto out;
+ }
+
+ vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+ if (!vreg) {
+ dev_err(dev, "No memory for %s regulator\n", name);
+ goto out;
+ }
+
+ vreg->name = kstrdup(name, GFP_KERNEL);
+
+ snprintf(prop_name, MAX_PROP_SIZE, "%s-max-microamp", name);
+ ret = of_property_read_u32(np, prop_name, &vreg->max_uA);
+ if (ret) {
+ dev_err(dev, "%s: unable to find %s err %d\n",
+ __func__, prop_name, ret);
+ goto out_free;
+ }
+
+ vreg->min_uA = 0;
+ if (!strcmp(name, "vcc")) {
+ if (o
[PATCH V3 16/16] scsi: ufs: definitions for phy interface
- Adding some of the definitions missing in unipro.h, including power
enumeration.
- Read Modify Write Line helper function
- Indication for the type of suspend
Signed-off-by: Dolev Raviv
Signed-off-by: Subhash Jadavani
Signed-off-by: Yaniv Gardi
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 33984cc..8b7c332 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4947,6 +4947,8 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
ret = ufshcd_suspend(hba, UFS_SYSTEM_PM);
out:
+ if (!ret)
+ hba->is_sys_suspended = true;
return ret;
}
EXPORT_SYMBOL(ufshcd_system_suspend);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index b94b835..d502d70 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -471,6 +471,7 @@ struct ufs_hba {
struct devfreq *devfreq;
struct ufs_clk_scaling clk_scaling;
+ bool is_sys_suspended;
};
/* Returns true if clocks can be gated. Otherwise false */
@@ -496,6 +497,23 @@ static inline bool
ufshcd_can_autobkops_during_suspend(struct ufs_hba *hba)
#define ufshcd_readl(hba, reg) \
readl((hba)->mmio_base + (reg))
+/**
+ * ufshcd_rmwl - read modify write into a register
+ * @hba - per adapter instance
+ * @mask - mask to apply on read value
+ * @val - actual value to write
+ * @reg - register address
+ */
+static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
+{
+ u32 tmp;
+
+ tmp = ufshcd_readl(hba, reg);
+ tmp &= ~mask;
+ tmp |= (val & mask);
+ ufshcd_writel(hba, tmp, reg);
+}
+
int ufshcd_alloc_host(struct device *, struct ufs_hba **);
int ufshcd_init(struct ufs_hba * , void __iomem * , unsigned int);
void ufshcd_remove(struct ufs_hba *);
diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h
index 0bb8041..3fc3e21 100644
--- a/drivers/scsi/ufs/unipro.h
+++ b/drivers/scsi/ufs/unipro.h
@@ -13,6 +13,44 @@
#define _UNIPRO_H_
/*
+ * M-TX Configuration Attributes
+ */
+#define TX_MODE0x0021
+#define TX_HSRATE_SERIES 0x0022
+#define TX_HSGEAR 0x0023
+#define TX_PWMGEAR 0x0024
+#define TX_AMPLITUDE 0x0025
+#define TX_HS_SLEWRATE 0x0026
+#define TX_SYNC_SOURCE 0x0027
+#define TX_HS_SYNC_LENGTH 0x0028
+#define TX_HS_PREPARE_LENGTH 0x0029
+#define TX_LS_PREPARE_LENGTH 0x002A
+#define TX_HIBERN8_CONTROL 0x002B
+#define TX_LCC_ENABLE 0x002C
+#define TX_PWM_BURST_CLOSURE_EXTENSION 0x002D
+#define TX_BYPASS_8B10B_ENABLE 0x002E
+#define TX_DRIVER_POLARITY 0x002F
+#define TX_HS_UNTERMINATED_LINE_DRIVE_ENABLE 0x0030
+#define TX_LS_TERMINATED_LINE_DRIVE_ENABLE 0x0031
+#define TX_LCC_SEQUENCER 0x0032
+#define TX_MIN_ACTIVATETIME0x0033
+#define TX_PWM_G6_G7_SYNC_LENGTH 0x0034
+
+/*
+ * M-RX Configuration Attributes
+ */
+#define RX_MODE0x00A1
+#define RX_HSRATE_SERIES 0x00A2
+#define RX_HSGEAR 0x00A3
+#define RX_PWMGEAR 0x00A4
+#define RX_LS_TERMINATED_ENABLE0x00A5
+#define RX_HS_UNTERMINATED_ENABLE 0x00A6
+#define RX_ENTER_HIBERN8 0x00A7
+#define RX_BYPASS_8B10B_ENABLE 0x00A8
+#define RX_TERMINATION_FORCE_ENABLE0x0089
+
+#define is_mphy_tx_attr(attr) (attr < RX_MODE)
+/*
* PHY Adpater attributes
*/
#define PA_ACTIVETXDATALANES 0x1560
@@ -87,6 +125,24 @@ enum {
PA_HS_MODE_B= 2,
};
+enum ufs_pwm_gear_tag {
+ UFS_PWM_DONT_CHANGE,/* Don't change Gear */
+ UFS_PWM_G1, /* PWM Gear 1 (default for reset) */
+ UFS_PWM_G2, /* PWM Gear 2 */
+ UFS_PWM_G3, /* PWM Gear 3 */
+ UFS_PWM_G4, /* PWM Gear 4 */
+ UFS_PWM_G5, /* PWM Gear 5 */
+ UFS_PWM_G6, /* PWM Gear 6 */
+ UFS_PWM_G7, /* PWM Gear 7 */
+};
+
+enum ufs_hs_gear_tag {
+ UFS_HS_DONT_CHANGE, /* Don't change Gear */
+ UFS_HS_G1, /* HS Gear 1 (default for reset) */
+ UFS_HS_G2, /* HS Gear 2 */
+ UFS_HS_G3, /* HS Gear 3 */
+};
+
/*
* Data Link Layer Attributes
*/
--
1.8.5.2
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info
[PATCH V3 11/16] scsi: ufs: refactor configuring power mode
Sometimes, the device shall report its maximum power and speed
capabilities, but we might not wish to configure it to use those
maximum capabilities.
This change adds support for the vendor specific host driver to
implement power change notify callback.
To enable configuring different power modes (number of lanes,
gear number and fast/slow modes) it is necessary to split the
configuration stage from the stage that reads the device max power mode.
In addition, it is not required to read the configuration more than
once, thus the configuration is stored after reading it once.
Signed-off-by: Dolev Raviv
Signed-off-by: Yaniv Gardi
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 79ef312..bfa9a75 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -178,6 +178,8 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int
tag);
static void ufshcd_hba_exit(struct ufs_hba *hba);
static int ufshcd_probe_hba(struct ufs_hba *hba);
static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
+static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
+ struct ufs_pa_layer_attr *desired_pwr_mode);
static inline void ufshcd_enable_irq(struct ufs_hba *hba)
{
@@ -1933,65 +1935,129 @@ static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
}
/**
- * ufshcd_config_max_pwr_mode - Set & Change power mode with
- * maximum capability attribute information.
- * @hba: per adapter instance
- *
- * Returns 0 on success, non-zero value on failure
+ * ufshcd_get_max_pwr_mode - reads the max power mode negotiated with device
+ * @hba: per-adapter instance
*/
-static int ufshcd_config_max_pwr_mode(struct ufs_hba *hba)
+static int ufshcd_get_max_pwr_mode(struct ufs_hba *hba)
{
- enum {RX = 0, TX = 1};
- u32 lanes[] = {1, 1};
- u32 gear[] = {1, 1};
- u8 pwr[] = {FASTAUTO_MODE, FASTAUTO_MODE};
- int ret;
+ struct ufs_pa_layer_attr *pwr_info = &hba->max_pwr_info.info;
+
+ if (hba->max_pwr_info.is_valid)
+ return 0;
+
+ pwr_info->pwr_tx = FASTAUTO_MODE;
+ pwr_info->pwr_rx = FASTAUTO_MODE;
+ pwr_info->hs_rate = PA_HS_MODE_B;
/* Get the connected lane count */
- ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &lanes[RX]);
- ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &lanes[TX]);
+ ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDRXDATALANES),
+ &pwr_info->lane_rx);
+ ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
+ &pwr_info->lane_tx);
+
+ if (!pwr_info->lane_rx || !pwr_info->lane_tx) {
+ dev_err(hba->dev, "%s: invalid connected lanes value. rx=%d,
tx=%d\n",
+ __func__,
+ pwr_info->lane_rx,
+ pwr_info->lane_tx);
+ return -EINVAL;
+ }
/*
* First, get the maximum gears of HS speed.
* If a zero value, it means there is no HSGEAR capability.
* Then, get the maximum gears of PWM speed.
*/
- ufshcd_dme_get(hba, UIC_ARG_MIB(PA_MAXRXHSGEAR), &gear[RX]);
- if (!gear[RX]) {
- ufshcd_dme_get(hba, UIC_ARG_MIB(PA_MAXRXPWMGEAR), &gear[RX]);
- pwr[RX] = SLOWAUTO_MODE;
+ ufshcd_dme_get(hba, UIC_ARG_MIB(PA_MAXRXHSGEAR), &pwr_info->gear_rx);
+ if (!pwr_info->gear_rx) {
+ ufshcd_dme_get(hba, UIC_ARG_MIB(PA_MAXRXPWMGEAR),
+ &pwr_info->gear_rx);
+ if (!pwr_info->gear_rx) {
+ dev_err(hba->dev, "%s: invalid max pwm rx gear read =
%d\n",
+ __func__, pwr_info->gear_rx);
+ return -EINVAL;
+ }
+ pwr_info->pwr_rx = SLOWAUTO_MODE;
}
- ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_MAXRXHSGEAR), &gear[TX]);
- if (!gear[TX]) {
+ ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_MAXRXHSGEAR),
+ &pwr_info->gear_tx);
+ if (!pwr_info->gear_tx) {
ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_MAXRXPWMGEAR),
- &gear[TX]);
- pwr[TX] = SLOWAUTO_MODE;
+ &pwr_info->gear_tx);
+ if (!pwr_info->gear_tx) {
+ dev_err(hba->dev, "%s: invalid max pwm tx gear read =
%d\n",
+ __func__, pwr_info->gear_tx);
+ return -EINVAL;
+ }
+ pwr_info->pwr_tx = SLOWAUTO_MODE;
}
+ hba->max_pwr_info.is_valid = true;
+ return 0;
+}
+
+/**
+ * ufshcd_config_pwr_mode - configure a new power mode
+ * @hba: per-adapter instance
+ * @desired_pwr_mode: desired power configuration
+ */
+static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
+ struct ufs_pa_layer_attr *desired_pwr_mode)
+{
+ struc
[PATCH V3 07/16] scsi: ufs: improve init sequence
From: Sujit Reddy Thumma
In ->hce_enable_notify() callback the vendor specific initialization
may carry out additional DME configuration using UIC commands and
hence the UIC command completion interrupt enable bit should be set
before the post reset notification.
Add retries if the link-startup fails. This is required since due to
hardware timing issues, the Uni-Pro link-startup might fail. The UFS
HCI recovery procedure contradicts the Uni-Pro sequence. The UFS HCI
specifies to resend DME_LINKSTARTUP command after IS.ULLS (link-lost
interrupt) is received. The Uni-Pro specifies that if link-startup
fails the link is in "down" state. The link-lost is indicated to the
DME user only when the link is up. Hence, the UFS HCI recovery procedure
of waiting for IS.ULLS and retrying link-startup may not work properly.
At the end, if detection fails, power off (disable clocks, regulators,
phy) if the UFS device detection fails. This saves power while UFS device
is not embedded into the system.
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 57a8dbb..1fabff4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -62,6 +62,12 @@
/* Task management command timeout */
#define TM_CMD_TIMEOUT 100 /* msecs */
+/* maximum number of link-startup retries */
+#define DME_LINKSTARTUP_RETRIES 3
+
+/* maximum number of reset retries before giving up */
+#define MAX_HOST_RESET_RETRIES 5
+
/* Expose the flag value from utp_upiu_query.value */
#define MASK_QUERY_UPIU_FLAG_LOC 0xFF
@@ -137,6 +143,8 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba);
static void ufshcd_async_scan(void *data, async_cookie_t cookie);
static int ufshcd_reset_and_restore(struct ufs_hba *hba);
static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
+static void ufshcd_hba_exit(struct ufs_hba *hba);
+static int ufshcd_probe_hba(struct ufs_hba *hba);
/*
* ufshcd_wait_for_register - wait for register value to change
@@ -2043,6 +2051,9 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
msleep(5);
}
+ /* enable UIC related interrupts */
+ ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+
if (hba->vops && hba->vops->hce_enable_notify)
hba->vops->hce_enable_notify(hba, POST_CHANGE);
@@ -2058,23 +2069,33 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
static int ufshcd_link_startup(struct ufs_hba *hba)
{
int ret;
+ int retries = DME_LINKSTARTUP_RETRIES;
- /* enable UIC related interrupts */
- ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+ do {
+ if (hba->vops && hba->vops->link_startup_notify)
+ hba->vops->link_startup_notify(hba, PRE_CHANGE);
- if (hba->vops && hba->vops->link_startup_notify)
- hba->vops->link_startup_notify(hba, PRE_CHANGE);
+ ret = ufshcd_dme_link_startup(hba);
- ret = ufshcd_dme_link_startup(hba);
- if (ret)
- goto out;
+ /* check if device is detected by inter-connect layer */
+ if (!ret && !ufshcd_is_device_present(hba)) {
+ dev_err(hba->dev, "%s: Device not present\n", __func__);
+ ret = -ENXIO;
+ goto out;
+ }
+
+ /*
+* DME link lost indication is only received when link is up,
+* but we can't be sure if the link is up until link startup
+* succeeds. So reset the local Uni-Pro and try again.
+*/
+ if (ret && ufshcd_hba_enable(hba))
+ goto out;
+ } while (ret && retries--);
- /* check if device is detected by inter-connect layer */
- if (!ufshcd_is_device_present(hba)) {
- dev_err(hba->dev, "%s: Device not present\n", __func__);
- ret = -ENXIO;
+ if (ret)
+ /* failed to get the link up... retire */
goto out;
- }
/* Include any host controller configuration via UIC commands */
if (hba->vops && hba->vops->link_startup_notify) {
@@ -3139,7 +3160,6 @@ out:
static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
{
int err;
- async_cookie_t cookie;
unsigned long flags;
/* Reset the host controller */
@@ -3152,10 +3172,9 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba
*hba)
goto out;
/* Establish the link again and restore the device */
- cookie = async_schedule(ufshcd_async_scan, hba);
- /* wait for async scan to be completed */
- async_synchronize_cookie(++cookie);
- if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
+ err = ufshcd_probe_hba(hba);
+
+ if (!err && (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL))
err = -EIO;
out:
if (err)
@@ -3177,8 +319
[PATCH V3 06/16] scsi: ufs: refactor query descriptor API support
From: Subhash Jadavani
Currently reading query descriptor is more tightened to each
descriptor type. This patch generalize the approach and allows
reading any parameter from any query descriptor.
Signed-off-by: Subhash Jadavani
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 729ce7d..7ea8e71 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -50,6 +50,8 @@
cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
(byte1 << 8) | (byte0))
+#define UFS_UPIU_MAX_GENERAL_LUN 8
+
/*
* UFS Protocol Information Unit related definitions
*/
@@ -129,10 +131,29 @@ enum desc_idn {
QUERY_DESC_IDN_RFU_1= 0x6,
QUERY_DESC_IDN_GEOMETRY = 0x7,
QUERY_DESC_IDN_POWER= 0x8,
- QUERY_DESC_IDN_RFU_2= 0x9,
+ QUERY_DESC_IDN_MAX,
+};
+
+enum desc_header_offset {
+ QUERY_DESC_LENGTH_OFFSET= 0x00,
+ QUERY_DESC_DESC_TYPE_OFFSET = 0x01,
+};
+
+enum ufs_desc_max_size {
+ QUERY_DESC_DEVICE_MAX_SIZE = 0x1F,
+ QUERY_DESC_CONFIGURAION_MAX_SIZE= 0x90,
+ QUERY_DESC_UNIT_MAX_SIZE= 0x23,
+ QUERY_DESC_INTERCONNECT_MAX_SIZE= 0x06,
+ /*
+* Max. 126 UNICODE characters (2 bytes per character) plus 2 bytes
+* of descriptor header.
+*/
+ QUERY_DESC_STRING_MAX_SIZE = 0xFE,
+ QUERY_DESC_GEOMETRY_MAZ_SIZE= 0x44,
+ QUERY_DESC_POWER_MAX_SIZE = 0x62,
+ QUERY_DESC_RFU_MAX_SIZE = 0x00,
};
-#define UNIT_DESC_MAX_SIZE 0x22
/* Unit descriptor parameters offsets in bytes*/
enum unit_desc_param {
UNIT_DESC_PARAM_LEN = 0x0,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b033702..57a8dbb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -78,6 +78,19 @@
_ret; \
})
+static u32 ufs_query_desc_max_size[] = {
+ QUERY_DESC_DEVICE_MAX_SIZE,
+ QUERY_DESC_CONFIGURAION_MAX_SIZE,
+ QUERY_DESC_UNIT_MAX_SIZE,
+ QUERY_DESC_RFU_MAX_SIZE,
+ QUERY_DESC_INTERCONNECT_MAX_SIZE,
+ QUERY_DESC_STRING_MAX_SIZE,
+ QUERY_DESC_RFU_MAX_SIZE,
+ QUERY_DESC_GEOMETRY_MAZ_SIZE,
+ QUERY_DESC_POWER_MAX_SIZE,
+ QUERY_DESC_RFU_MAX_SIZE,
+};
+
enum {
UFSHCD_MAX_CHANNEL = 0,
UFSHCD_MAX_ID = 1,
@@ -124,8 +137,6 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba);
static void ufshcd_async_scan(void *data, async_cookie_t cookie);
static int ufshcd_reset_and_restore(struct ufs_hba *hba);
static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
-static int ufshcd_read_sdev_qdepth(struct ufs_hba *hba,
- struct scsi_device *sdev);
/*
* ufshcd_wait_for_register - wait for register value to change
@@ -1393,6 +1404,115 @@ out:
}
/**
+ * ufshcd_read_desc_param - read the specified descriptor parameter
+ * @hba: Pointer to adapter instance
+ * @desc_id: descriptor idn value
+ * @desc_index: descriptor index
+ * @param_offset: offset of the parameter to read
+ * @param_read_buf: pointer to buffer where parameter would be read
+ * @param_size: sizeof(param_read_buf)
+ *
+ * Return 0 in case of success, non-zero otherwise
+ */
+static int ufshcd_read_desc_param(struct ufs_hba *hba,
+ enum desc_idn desc_id,
+ int desc_index,
+ u32 param_offset,
+ u8 *param_read_buf,
+ u32 param_size)
+{
+ int ret;
+ u8 *desc_buf;
+ u32 buff_len;
+ bool is_kmalloc = true;
+
+ /* safety checks */
+ if (desc_id >= QUERY_DESC_IDN_MAX)
+ return -EINVAL;
+
+ buff_len = ufs_query_desc_max_size[desc_id];
+ if ((param_offset + param_size) > buff_len)
+ return -EINVAL;
+
+ if (!param_offset && (param_size == buff_len)) {
+ /* memory space already available to hold full descriptor */
+ desc_buf = param_read_buf;
+ is_kmalloc = false;
+ } else {
+ /* allocate memory to hold full descriptor */
+ desc_buf = kmalloc(buff_len, GFP_KERNEL);
+ if (!desc_buf)
+ return -ENOMEM;
+ }
+
+ ret = ufshcd_query_descriptor(hba, UPIU_QUERY_OPCODE_READ_DESC,
+ desc_id, desc_index, 0, desc_buf,
+ &buff_len);
+
+ if (ret || (buff_len < ufs_query_desc_max_size[desc_id]) ||
+ (desc_buf[QUERY_DESC_LENGTH_OFFSET] !=
+ufs_query_desc_max_size[desc_id])
+ || (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id))
[PATCH V3 05/16] scsi: ufs: Add clock initialization support
From: Sujit Reddy Thumma
Add generic clock initialization support for UFSHCD platform
driver. The clock info is read from device tree using standard
clock bindings. A generic max-clock-frequency-hz property is
defined to save information on maximum operating clock frequency
the h/w supports.
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index 65e3117..b0f791a 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -21,8 +21,17 @@ Optional properties:
- vccq-max-microamp : specifies max. load that can be drawn from vccq
supply
- vccq2-max-microamp: specifies max. load that can be drawn from vccq2
supply
+- clocks: List of phandle and clock specifier pairs
+- clock-names : List of clock input name strings sorted in the same
+ order as the clocks property.
+- max-clock-frequency-hz : List of maximum operating frequency stored in the
same
+ order as the clocks property. If this property is
not
+ defined or a value in the array is "0" then it is
assumed
+ that the frequency is set by the parent clock or a
+ fixed rate clock source.
+
Note: If above properties are not defined it can be assumed that the supply
-regulators are always on.
+regulators or clocks are always on.
Example:
ufshc@0xfc598000 {
@@ -37,4 +46,8 @@ Example:
vcc-max-microamp = 50;
vccq-max-microamp = 20;
vccq2-max-microamp = 20;
+
+ clocks = <&core 0>, <&ref 0>, <&iface 0>;
+ clock-names = "core_clk", "ref_clk", "iface_clk";
+ max-clock-frequency-hz = <1 1920 0>;
};
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 7a6edbc..2a26faa 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -170,6 +170,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
return err;
}
+ INIT_LIST_HEAD(&hba->clk_list_head);
+
err = ufshcd_init(hba, mmio_base, pdev->irq);
if (err) {
dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 51e47c4..642d80f 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -53,6 +53,71 @@ static struct ufs_hba_variant_ops *get_variant_ops(struct
device *dev)
return NULL;
}
+static int ufshcd_parse_clock_info(struct ufs_hba *hba)
+{
+ int ret = 0;
+ int cnt;
+ int i;
+ struct device *dev = hba->dev;
+ struct device_node *np = dev->of_node;
+ char *name;
+ u32 *clkfreq = NULL;
+ struct ufs_clk_info *clki;
+
+ if (!np)
+ goto out;
+
+ INIT_LIST_HEAD(&hba->clk_list_head);
+
+ cnt = of_property_count_strings(np, "clock-names");
+ if (!cnt || (cnt == -EINVAL)) {
+ dev_info(dev, "%s: Unable to find clocks, assuming enabled\n",
+ __func__);
+ } else if (cnt < 0) {
+ dev_err(dev, "%s: count clock strings failed, err %d\n",
+ __func__, cnt);
+ ret = cnt;
+ }
+
+ if (cnt <= 0)
+ goto out;
+
+ clkfreq = kzalloc(cnt * sizeof(*clkfreq), GFP_KERNEL);
+ if (!clkfreq) {
+ ret = -ENOMEM;
+ dev_err(dev, "%s: memory alloc failed\n", __func__);
+ goto out;
+ }
+
+ ret = of_property_read_u32_array(np,
+ "max-clock-frequency-hz", clkfreq, cnt);
+ if (ret && (ret != -EINVAL)) {
+ dev_err(dev, "%s: invalid max-clock-frequency-hz property,
%d\n",
+ __func__, ret);
+ goto out;
+ }
+
+ for (i = 0; i < cnt; i++) {
+ ret = of_property_read_string_index(np,
+ "clock-names", i, (const char **)&name);
+ if (ret)
+ goto out;
+
+ clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
+ if (!clki) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ clki->max_freq = clkfreq[i];
+ clki->name = kstrdup(name, GFP_KERNEL);
+ list_add_tail(&clki->list, &hba->clk_list_head);
+ }
+out:
+ kfree(clkfreq);
+ return ret;
+}
+
#define MAX_PROP_SIZE 32
static int ufshcd_populate_vreg(struct device *dev, const char *name,
struct ufs_vreg **out_vreg)
@@ -266,6 +331,12 @@ static int ufshcd_pltfrm_p
[PATCH V3 08/16] scsi: ufs: Active Power Mode - configuring bActiveICCLevel
From: Yaniv Gardi
The maximum power consumption in active is determined by bActiveICCLevel.
The configuration is done by reading max current supported by the
regulators connected to VCC, VCCQ and VCCQ2 rails on the boards, and
reading the current consumption levels from the device for each rails
(vcc/vccq/vccq2) using power descriptor.
We configure the bActiveICCLevel attribute, with the max value that
correspond to the minimum-of(VCC-current-level,VCCQ-current-level,
VCCQ2-current-level).
In order to minimize resume latency, pre-fetch icc levels and reference
clock during initialization and avoid reading them each link startup
during resume.
Signed-off-by: Raviv Shvili
Signed-off-by: Yaniv Gardi
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 7ea8e71..b0e1f62 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -115,6 +115,7 @@ enum flag_idn {
/* Attribute idn for Query requests */
enum attr_idn {
+ QUERY_ATTR_IDN_ACTIVE_ICC_LVL = 0x03,
QUERY_ATTR_IDN_BKOPS_STATUS = 0x05,
QUERY_ATTR_IDN_EE_CONTROL = 0x0D,
QUERY_ATTR_IDN_EE_STATUS= 0x0E,
@@ -174,6 +175,31 @@ enum unit_desc_param {
UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22,
};
+/* bActiveICCLevel parameter current units */
+enum {
+ UFSHCD_NANO_AMP = 0,
+ UFSHCD_MICRO_AMP= 1,
+ UFSHCD_MILI_AMP = 2,
+ UFSHCD_AMP = 3,
+};
+
+#define POWER_DESC_MAX_SIZE0x62
+#define POWER_DESC_MAX_ACTV_ICC_LVLS 16
+
+/* Attribute bActiveICCLevel parameter bit masks definitions */
+#define ATTR_ICC_LVL_UNIT_OFFSET 14
+#define ATTR_ICC_LVL_UNIT_MASK (0x3 << ATTR_ICC_LVL_UNIT_OFFSET)
+#define ATTR_ICC_LVL_VALUE_MASK0x3FF
+
+/* Power descriptor parameters offsets in bytes */
+enum power_desc_param_offset {
+ PWR_DESC_LEN= 0x0,
+ PWR_DESC_TYPE = 0x1,
+ PWR_DESC_ACTIVE_LVLS_VCC_0 = 0x2,
+ PWR_DESC_ACTIVE_LVLS_VCCQ_0 = 0x22,
+ PWR_DESC_ACTIVE_LVLS_VCCQ2_0= 0x42,
+};
+
/* Exception event mask values */
enum {
MASK_EE_STATUS = 0x,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1fabff4..a851323 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3265,6 +3265,125 @@ static int ufshcd_eh_host_reset_handler(struct
scsi_cmnd *cmd)
}
/**
+ * ufshcd_get_max_icc_level - calculate the ICC level
+ * @sup_curr_uA: max. current supported by the regulator
+ * @start_scan: row at the desc table to start scan from
+ * @buff: power descriptor buffer
+ *
+ * Returns calculated max ICC level for specific regulator
+ */
+static u32 ufshcd_get_max_icc_level(int sup_curr_uA, u32 start_scan, char
*buff)
+{
+ int i;
+ int curr_uA;
+ u16 data;
+ u16 unit;
+
+ for (i = start_scan; i >= 0; i--) {
+ data = be16_to_cpu(*((u16 *)(buff + 2*i)));
+ unit = (data & ATTR_ICC_LVL_UNIT_MASK) >>
+ ATTR_ICC_LVL_UNIT_OFFSET;
+ curr_uA = data & ATTR_ICC_LVL_VALUE_MASK;
+ switch (unit) {
+ case UFSHCD_NANO_AMP:
+ curr_uA = curr_uA / 1000;
+ break;
+ case UFSHCD_MILI_AMP:
+ curr_uA = curr_uA * 1000;
+ break;
+ case UFSHCD_AMP:
+ curr_uA = curr_uA * 1000 * 1000;
+ break;
+ case UFSHCD_MICRO_AMP:
+ default:
+ break;
+ }
+ if (sup_curr_uA >= curr_uA)
+ break;
+ }
+ if (i < 0) {
+ i = 0;
+ pr_err("%s: Couldn't find valid icc_level = %d", __func__, i);
+ }
+
+ return (u32)i;
+}
+
+/**
+ * ufshcd_calc_icc_level - calculate the max ICC level
+ * In case regulators are not initialized we'll return 0
+ * @hba: per-adapter instance
+ * @desc_buf: power descriptor buffer to extract ICC levels from.
+ * @len: length of desc_buff
+ *
+ * Returns calculated ICC level
+ */
+static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
+ u8 *desc_buf, int len)
+{
+ u32 icc_level = 0;
+
+ if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
+ !hba->vreg_info.vccq2) {
+ dev_err(hba->dev,
+ "%s: Regulator capability was not set, actvIccLevel=%d",
+ __func__, icc_level);
+ goto out;
+ }
+
+ if (hba->vreg_info.vcc)
+ icc_level = ufshcd_get_max_icc_level(
+ hba->vreg_info.vcc->max_uA,
+
[PATCH V3 03/16] scsi: ufs: Allow vendor specific initialization
From: Sujit Reddy Thumma
Some vendor specific controller versions might need to configure
vendor specific - registers, clocks, voltage regulators etc. to
initialize the host controller UTP layer and Uni-Pro stack.
Provide some common initialization operations that can be used
to configure vendor specifics. The methods can be extended in
future, for example, for power mode transitions.
The operations are vendor/board specific and hence determined with
the help of compatible property in device tree.
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index afaabe2..7a6edbc 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -164,7 +164,13 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
mmio_base = pcim_iomap_table(pdev)[0];
- err = ufshcd_init(&pdev->dev, &hba, mmio_base, pdev->irq);
+ err = ufshcd_alloc_host(&pdev->dev, &hba);
+ if (err) {
+ dev_err(&pdev->dev, "Allocation failed\n");
+ return err;
+ }
+
+ err = ufshcd_init(hba, mmio_base, pdev->irq);
if (err) {
dev_err(&pdev->dev, "Initialization failed\n");
return err;
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 5e46232..d727b1a 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -35,9 +35,24 @@
#include
#include
+#include
#include "ufshcd.h"
+static const struct of_device_id ufs_of_match[];
+static struct ufs_hba_variant_ops *get_variant_ops(struct device *dev)
+{
+ if (dev->of_node) {
+ const struct of_device_id *match;
+
+ match = of_match_node(ufs_of_match, dev->of_node);
+ if (match)
+ return (struct ufs_hba_variant_ops *)match->data;
+ }
+
+ return NULL;
+}
+
#ifdef CONFIG_PM
/**
* ufshcd_pltfrm_suspend - suspend power management function
@@ -138,8 +153,8 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
mmio_base = devm_ioremap_resource(dev, mem_res);
- if (IS_ERR(mmio_base)) {
- err = PTR_ERR(mmio_base);
+ if (IS_ERR(*(void **)&mmio_base)) {
+ err = PTR_ERR(*(void **)&mmio_base);
goto out;
}
@@ -150,10 +165,18 @@ static int ufshcd_pltfrm_probe(struct platform_device
*pdev)
goto out;
}
+ err = ufshcd_alloc_host(dev, &hba);
+ if (err) {
+ dev_err(&pdev->dev, "Allocation failed\n");
+ goto out;
+ }
+
+ hba->vops = get_variant_ops(&pdev->dev);
+
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
- err = ufshcd_init(dev, &hba, mmio_base, irq);
+ err = ufshcd_init(hba, mmio_base, irq);
if (err) {
dev_err(dev, "Intialization failed\n");
goto out_disable_rpm;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ba27215..d0565b0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3,6 +3,7 @@
*
* This code is based on drivers/scsi/ufs/ufshcd.c
* Copyright (C) 2011-2013 Samsung India Software Operations
+ * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
*
* Authors:
* Santosh Yaraganavi
@@ -31,6 +32,9 @@
* circumstances will the contributor of this Program be liable for
* any damages of any kind arising from your use or distribution of
* this program.
+ *
+ * The Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
*/
#include
@@ -175,13 +179,14 @@ static inline u32 ufshcd_get_ufs_version(struct ufs_hba
*hba)
/**
* ufshcd_is_device_present - Check if any device connected to
* the host controller
- * @reg_hcs - host controller status register value
+ * @hba: pointer to adapter instance
*
* Returns 1 if device present, 0 if no device detected
*/
-static inline int ufshcd_is_device_present(u32 reg_hcs)
+static inline int ufshcd_is_device_present(struct ufs_hba *hba)
{
- return (DEVICE_PRESENT & reg_hcs) ? 1 : 0;
+ return (ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
+ DEVICE_PRESENT) ? 1 : 0;
}
/**
@@ -1798,11 +1803,10 @@ out:
* @hba: per adapter instance
*
* To bring UFS host controller to operational state,
- * 1. Check if device is present
- * 2. Enable required interrupts
- * 3. Configure interrupt aggregation
- * 4. Program UTRL and UTMRL base addres
- * 5. Configure run-stop-registers
+ * 1. Enable required interrupts
+ * 2. Configure interrupt aggregation
+ * 3. Program UTRL and UTMRL base addres
+ * 4. Configure run-stop-registers
*
* Returns 0 on suc
[PATCH V3 01/16] scsi: support well known logical units
From: Subhash Jadavani
REPORT LUNS command has "SELECT REPORT" field which controls what type of
logical units to be reported by device server. According to UFS device
standard, if this field is set to 0, REPORT LUNS would report only report
standard logical units. If it's set to 1 then it would report only well
known logical unit and if it's set to 2 then device would report both
standard and well known logical units.
Signed-off-by: Subhash Jadavani
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 56675db..d7f0df8 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -805,6 +805,14 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned
char *inq_result,
} else {
sdev->type = (inq_result[0] & 0x1f);
sdev->removable = (inq_result[1] & 0x80) >> 7;
+
+ /*
+* some devices may respond with wrong type for
+* well-known logical units. Force well-known type
+* to enumerate them correctly.
+*/
+ if (scsi_is_wlun(sdev->lun))
+ sdev->type = TYPE_WLUN;
}
if (sdev->type == TYPE_RBC || sdev->type == TYPE_ROM) {
@@ -1400,6 +1408,12 @@ static int scsi_report_lun_scan(struct scsi_target
*starget, int bflags,
memset(&scsi_cmd[1], 0, 5);
/*
+* Set "SELECT REPORT" field to 0x2 which will make device to
+* report well known logical units along with standard LUs.
+*/
+ scsi_cmd[2] = 0x2;
+
+ /*
* bytes 6 - 9: length of the command.
*/
scsi_cmd[6] = (unsigned char) (length >> 24) & 0xff;
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 261e708..d17178e 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -333,6 +333,7 @@ static inline int scsi_status_is_good(int status)
#define TYPE_RBC 0x0e
#define TYPE_OSD0x11
#define TYPE_ZBC0x14
+#define TYPE_WLUN 0x1e/* well-known logical unit */
#define TYPE_NO_LUN 0x7f
/* SCSI protocols; these are taken from SPC-3 section 7.5 */
--
1.8.5.2
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 02/16] scsi: balance out autopm get/put calls in scsi_sysfs_add_sdev()
From: Subhash Jadavani SCSI Well-known logical units generally don't have any scsi driver associated with it which means no one will call scsi_autopm_put_device() on these wlun scsi devices and this would result in keeping the corresponding scsi device always active (hence LLD can't be suspended as well). Same exact problem can be seen for other scsi device representing normal logical unit whose driver is yet to be loaded. This patch fixes the above problem with this approach: - make the scsi_autopm_put_device call at the end of scsi_sysfs_add_sdev to make it balance out the get earlier in the function. - let drivers do paired get/put calls in their probe methods. Signed-off-by: Subhash Jadavani Signed-off-by: Dolev Raviv diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 8b4105a..3524b68 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1044,10 +1044,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) pm_runtime_enable(&sdev->sdev_gendev); scsi_autopm_put_target(starget); - /* The following call will keep sdev active indefinitely, until -* its driver does a corresponding scsi_autopm_pm_device(). Only -* drivers supporting autosuspend will do this. -*/ scsi_autopm_get_device(sdev); error = device_add(&sdev->sdev_gendev); @@ -1085,6 +1081,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) } } + scsi_autopm_put_device(sdev); return error; } diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index aa43496..0cb5c9f 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2965,6 +2965,7 @@ static int sd_probe(struct device *dev) int index; int error; + scsi_autopm_get_device(sdp); error = -ENODEV; if (sdp->type != TYPE_DISK && sdp->type != TYPE_MOD && sdp->type != TYPE_RBC) goto out; @@ -3041,6 +3042,7 @@ static int sd_probe(struct device *dev) out_free: kfree(sdkp); out: + scsi_autopm_put_device(sdp); return error; } diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 7eeb936..2de44cc 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -657,6 +657,7 @@ static int sr_probe(struct device *dev) struct scsi_cd *cd; int minor, error; + scsi_autopm_get_device(sdev); error = -ENODEV; if (sdev->type != TYPE_ROM && sdev->type != TYPE_WORM) goto fail; @@ -744,6 +745,7 @@ fail_put: fail_free: kfree(cd); fail: + scsi_autopm_put_device(sdev); return error; } diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c index aff9689..d3fd6e8 100644 --- a/drivers/scsi/st.c +++ b/drivers/scsi/st.c @@ -4105,6 +4105,7 @@ static int st_probe(struct device *dev) return -ENODEV; } + scsi_autopm_get_device(SDp); i = queue_max_segments(SDp->request_queue); if (st_max_sg_segs < i) i = st_max_sg_segs; @@ -4244,6 +4245,7 @@ out_put_disk: out_buffer_free: kfree(buffer); out: + scsi_autopm_put_device(SDp); return -ENODEV; }; -- 1.8.5.2 -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 00/16] UFS: Power management support
This patch seies introduces support for power management in the driver as well as vendor specific initialization - registers, clocks, voltage regulators etc. It includes also a rework for the init sequence and other PM pre-requisite such as write protection support, handling well-known LUN, error handling (retries), bkops, START_STOP unit command, and ICC levels settings. -- Changes from V2: - Reordered scsi core patches - add patch "[PATCH V3 02/16] scsi: balance out autopm get/put calls in" - Minor changes/fixes to the patches: * "[PATCH V3 10/16] scsi: ufs: add UFS power management support" * "[PATCH V3 12/16] scsi: ufs: Add support for clock gating" * "[PATCH V3 16/16] scsi: ufs: definitions for phy interface" In order to address community concerns, and as a result of further development and testing. Changes from V1: - 6 new patches apended at the end - Allow overriding power configuration with controller support and preferences/capabilities - Allow overriding power choice with controller capabilities - Add support for clock gating and clock scaling - Add capability to control the auto bkops during suspend - Add misc changes for phy/unipro driver usage Dolev Raviv (2): scsi: ufs: refactor configuring power mode scsi: ufs: definitions for phy interface Sahitya Tummala (3): scsi: ufs: Add support for clock gating scsi: ufs: Add freq-table-hz property for UFS device scsi: ufs: Add support for clock scaling using devfreq framework Subhash Jadavani (6): scsi: support well known logical units scsi: balance out autopm get/put calls in scsi_sysfs_add_sdev() scsi: ufs: refactor query descriptor API support scsi: ufs: introduce well known logical unit in ufs scsi: ufs: add UFS power management support scsi: ufs: tune bkops while power managment events Sujit Reddy Thumma (4): scsi: ufs: Allow vendor specific initialization scsi: ufs: Add regulator enable support scsi: ufs: Add clock initialization support scsi: ufs: improve init sequence Yaniv Gardi (1): scsi: ufs: Active Power Mode - configuring bActiveICCLevel .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 37 + drivers/scsi/scsi_scan.c | 14 + drivers/scsi/scsi_sysfs.c |5 +- drivers/scsi/sd.c |2 + drivers/scsi/sr.c |2 + drivers/scsi/st.c |2 + drivers/scsi/ufs/Kconfig |2 + drivers/scsi/ufs/ufs.h | 131 +- drivers/scsi/ufs/ufshcd-pci.c | 55 +- drivers/scsi/ufs/ufshcd-pltfrm.c | 282 ++- drivers/scsi/ufs/ufshcd.c | 2408 ++-- drivers/scsi/ufs/ufshcd.h | 277 ++- drivers/scsi/ufs/ufshci.h |9 +- drivers/scsi/ufs/unipro.h | 56 + include/scsi/scsi.h|1 + 15 files changed, 2962 insertions(+), 321 deletions(-) -- 1.8.5.2 -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/21] uas: Do not use scsi_host_find_tag
Using scsi_host_find_tag with tags returned by the device is unsafe for
multiple reasons:
1) It returns tags->rqs[tag], which may be non NULL even when the cmnd is
not owned by us
2) It returns tags->rqs[tag], without holding any locks protecting it
3) It returns tags->rqs[tag], without doing any boundary checking
Instead keep our own list which maps tags -> inflight cmnds.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 39 ++-
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 05c1c81..b2423d3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -29,6 +29,8 @@
#include "uas-detect.h"
+#define MAX_CMNDS 256
+
/*
* The r00-r01c specs define this version of the SENSE IU data structure.
* It's still in use by several different firmware releases.
@@ -54,7 +56,7 @@ struct uas_dev_info {
unsigned use_streams:1;
unsigned uas_sense_old:1;
unsigned shutdown:1;
- struct scsi_cmnd *cmnd;
+ struct scsi_cmnd *cmnd[MAX_CMNDS];
spinlock_t lock;
struct work_struct work;
struct list_head inflight_list;
@@ -314,6 +316,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
if (cmdinfo->state & COMMAND_ABORTED)
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
list_del(&cmdinfo->list);
+ devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
cmnd->scsi_done(cmnd);
return 0;
}
@@ -339,7 +342,7 @@ static void uas_stat_cmplt(struct urb *urb)
struct scsi_cmnd *cmnd;
struct uas_cmd_info *cmdinfo;
unsigned long flags;
- u16 tag;
+ unsigned int idx;
spin_lock_irqsave(&devinfo->lock, flags);
@@ -357,21 +360,17 @@ static void uas_stat_cmplt(struct urb *urb)
goto out;
}
- tag = be16_to_cpup(&iu->tag) - 1;
- if (tag == 0)
- cmnd = devinfo->cmnd;
- else
- cmnd = scsi_host_find_tag(shost, tag - 1);
-
- if (!cmnd)
+ idx = be16_to_cpup(&iu->tag) - 1;
+ if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
+ dev_err(&urb->dev->dev,
+ "stat urb: no pending cmd for tag %d\n", idx + 1);
goto out;
+ }
+ cmnd = devinfo->cmnd[idx];
cmdinfo = (void *)&cmnd->SCp;
switch (iu->iu_id) {
case IU_ID_STATUS:
- if (devinfo->cmnd == cmnd)
- devinfo->cmnd = NULL;
-
if (urb->actual_length < 16)
devinfo->uas_sense_old = 1;
if (devinfo->uas_sense_old)
@@ -672,6 +671,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
struct uas_dev_info *devinfo = sdev->hostdata;
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
unsigned long flags;
+ unsigned int stream;
int err;
BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
@@ -685,19 +685,16 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
return 0;
}
- if (devinfo->cmnd) {
+ stream = uas_get_tag(cmnd);
+ if (devinfo->cmnd[stream - 1]) {
spin_unlock_irqrestore(&devinfo->lock, flags);
return SCSI_MLQUEUE_DEVICE_BUSY;
}
- memset(cmdinfo, 0, sizeof(*cmdinfo));
-
- if (!blk_rq_tagged(cmnd->request))
- devinfo->cmnd = cmnd;
-
cmnd->scsi_done = done;
- cmdinfo->stream = uas_get_tag(cmnd);
+ memset(cmdinfo, 0, sizeof(*cmdinfo));
+ cmdinfo->stream = stream;
cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
switch (cmnd->sc_data_direction) {
@@ -727,6 +724,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
uas_add_work(cmdinfo);
}
+ devinfo->cmnd[stream - 1] = cmnd;
list_add_tail(&cmdinfo->list, &devinfo->inflight_list);
spin_unlock_irqrestore(&devinfo->lock, flags);
return 0;
@@ -862,7 +860,6 @@ static int uas_configure_endpoints(struct uas_dev_info
*devinfo)
int r;
devinfo->uas_sense_old = 0;
- devinfo->cmnd = NULL;
r = uas_find_endpoints(devinfo->intf->cur_altsetting, eps);
if (r)
@@ -882,7 +879,7 @@ static int uas_configure_endpoints(struct uas_dev_info
*devinfo)
devinfo->use_streams = 0;
} else {
devinfo->qdepth = usb_alloc_streams(devinfo->intf, eps + 1,
- 3, 256, GFP_NOIO);
+ 3, MAX_CMNDS, GFP_NOIO);
if (devinfo->qdepth < 0)
return devinfo->qdepth;
devinfo->use_streams = 1;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body
[PATCH 08/21] uas: Free data urbs on completion
Now that we no longer drop our lock to unlink the data urbs, we can simply
free them on completion, making their handling consistent with the other urbs.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b91b5e9..4c7d337 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -286,8 +286,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
return -EBUSY;
WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
- usb_free_urb(cmdinfo->data_in_urb);
- usb_free_urb(cmdinfo->data_out_urb);
if (cmdinfo->state & COMMAND_ABORTED)
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
list_del(&cmdinfo->list);
@@ -416,9 +414,11 @@ static void uas_data_cmplt(struct urb *urb)
if (cmdinfo->data_in_urb == urb) {
sdb = scsi_in(cmnd);
cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
+ cmdinfo->data_in_urb = NULL;
} else if (cmdinfo->data_out_urb == urb) {
sdb = scsi_out(cmnd);
cmdinfo->state &= ~DATA_OUT_URB_INFLIGHT;
+ cmdinfo->data_out_urb = NULL;
}
if (sdb == NULL) {
WARN_ON_ONCE(1);
@@ -448,6 +448,7 @@ static void uas_data_cmplt(struct urb *urb)
}
uas_try_complete(cmnd, __func__);
out:
+ usb_free_urb(urb);
spin_unlock_irqrestore(&devinfo->lock, flags);
}
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
Hi,
Note this series is NOT intended for stable, but I accidentally
had "cc = sta...@vger.kernel.org" in my .git/config when sending
this series, please ignore for stable.
NACK for stable.
Sorry & Regards,
Hans
On 09/10/2014 01:46 PM, Hans de Goede wrote:
> From: Sanjeev Sharma
>
> On some architecture spin_is_locked() always return false in
> uniprocessor configuration and therefore it would be advise to replace
> with lockdep_assert_held().
>
> Signed-off-by: Sanjeev Sharma
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/storage/uas.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..05b2d8e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info
> *devinfo,
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>
> uas_log_cmd_state(cmnd, caller);
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
> cmdinfo->state |= COMMAND_ABORTED;
> cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
> struct uas_dev_info *devinfo = cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> cmdinfo->state |= IS_IN_WORK_LIST;
> schedule_work(&devinfo->work);
> }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
> char *caller)
> struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & (COMMAND_INFLIGHT |
> DATA_IN_URB_INFLIGHT |
> DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
> struct urb *urb;
> int err;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & SUBMIT_STATUS_URB) {
> urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
> if (!urb)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/21] uas: zap_pending: data urbs should have completed at this time
The data urbs are all killed before calling zap_pending, and their completion handler should have cleared their inflight flag. Do not 0 the data inflight flags, and add a check for try_complete succeeding, as it should always succeed when called from zap_pending. Signed-off-by: Hans de Goede --- drivers/usb/storage/uas.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index bba5498..ec6547d 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -150,12 +150,10 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result) struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp); uas_log_cmd_state(cmnd, __func__); - /* all urbs are killed, clear inflight bits */ - cmdinfo->state &= ~(COMMAND_INFLIGHT | - DATA_IN_URB_INFLIGHT | - DATA_OUT_URB_INFLIGHT); + /* Sense urbs were killed, clear COMMAND_INFLIGHT manually */ + cmdinfo->state &= ~COMMAND_INFLIGHT; cmnd->result = result << 16; - uas_try_complete(cmnd, __func__); + WARN_ON(uas_try_complete(cmnd, __func__) != 0); } spin_unlock_irqrestore(&devinfo->lock, flags); } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/21] uas: Remove task-management / abort error handling code
There are various bug reports about oopses / hangs with the uas driver,
which all point to the abort-command and logical-unit-reset (task-management)
error handling paths.
Getting these right is very hard, there are quite a few corner cases, and
testing is almost impossible since under normal operation these code paths
are not used at all.
Another problem is that there are also some cases where it simply is not clear
what to do at all. E.g. over usb-2 multiple outstanding commands share the same
endpoint. What if a command gets aborted while its sense urb is half way
through completing (so some data has been transfered but not all). Since the
urb is not yet complete we don't know if the sense urb is actually for this
command, or for one of the other oustanding commands. If it is for one of the
other commands and we cancel it, then we end up in an undefined state. But if
it is actually for the command we're aborting, and the abort succeeds, then it
may never complete...
This exact same problem applies to logical unit resets too, if there are
multiple luns, then commands outstanding on both luns share the sense
endpoint. If there is only a single lun, then doing a logical unit reset is
little better then doing a full usb device reset.
So summarizing because:
1) abort / lun-reset is very tricky to get right
2) Not being able to test the tricky code, which means it will have bugs
3) This being a code path which under normal operation will never happen,
so being slow / sub-optimal here is not really an issue
4) Under error conditions we will still be able to recover through usb
device resets.
5) This may be a bit slower in some cases, but this is actually faster in
cases where the bridge ship has locked up, which seems to be the most
common error case sofar.
This commit removes the abort / lun-reset error handling paths, and also the
taks-mgmt code since those are the only 2 task-mgmt users. Leaving only the
(tested and testable) usb-device-reset error handling path in place.
Note I realize that this is somewhat of a big hammer, but currently people
are seeing very hard to debug oopses with uas. First let focus on making uas
work reliable, then we can later look into adding more fine grained error
handling.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 177 +-
1 file changed, 1 insertion(+), 176 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 05b2d8e..307658d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -2,7 +2,7 @@
* USB Attached SCSI
* Note that this is not the same as the USB Mass Storage driver
*
- * Copyright Hans de Goede for Red Hat, Inc. 2013
+ * Copyright Hans de Goede for Red Hat, Inc. 2013 - 2014
* Copyright Matthew Wilcox for Intel Corp, 2010
* Copyright Sarah Sharp for Intel Corp, 2010
*
@@ -50,11 +50,9 @@ struct uas_dev_info {
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
int qdepth, resetting;
- struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
unsigned use_streams:1;
unsigned uas_sense_old:1;
- unsigned running_task:1;
unsigned shutdown:1;
struct scsi_cmnd *cmnd;
spinlock_t lock;
@@ -205,7 +203,6 @@ static void uas_zap_dead(struct uas_dev_info *devinfo)
DATA_OUT_URB_INFLIGHT);
uas_try_complete(cmnd, __func__);
}
- devinfo->running_task = 0;
spin_unlock_irqrestore(&devinfo->lock, flags);
}
@@ -348,13 +345,6 @@ static void uas_stat_cmplt(struct urb *urb)
cmnd = scsi_host_find_tag(shost, tag - 1);
if (!cmnd) {
- if (iu->iu_id == IU_ID_RESPONSE) {
- if (!devinfo->running_task)
- dev_warn(&urb->dev->dev,
- "stat urb: recv unexpected response iu\n");
- /* store results for uas_eh_task_mgmt() */
- memcpy(&devinfo->response, iu,
sizeof(devinfo->response));
- }
usb_free_urb(urb);
spin_unlock_irqrestore(&devinfo->lock, flags);
return;
@@ -534,56 +524,6 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
return NULL;
}
-static int uas_submit_task_urb(struct scsi_cmnd *cmnd, gfp_t gfp,
- u8 function, u16 stream_id)
-{
- struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
- struct usb_device *udev = devinfo->udev;
- struct urb *urb = usb_alloc_urb(0, gfp);
- struct task_mgmt_iu *iu;
- int err = -ENOMEM;
-
- if (!urb)
- goto err;
-
- iu = kzalloc(sizeof(*iu), gfp);
- if (!iu)
- goto err;
-
- iu->iu_id = IU_ID_TASK_MGMT;
- iu->tag = cpu_to_be16(st
[PATCH 00/21] uas: rewrite error handling for robustness + misc cleanups
Hi Greg, et al, Since we've been receiving multiple bug reports with crashes / oopses related to uas error handling, I've spend the last 7 days rewriting the error handling code. This new code has been extensively tested, doing externally triggered usb-device-resets and scsi bus resets while having multiple io streams active. It has all seen some serious shakedown in my attempts to get the ASM1051 to work, which would throw all kind of fun errors. Without this series (and without the blacklist) connecting an ASM1051 device causes a lockup 30 seconds after plug-in (so when the first scsi timeout fires), with this series it goes into error handling (*) for ages before finding the disk (and then some more with some disks), but it actually works somewhat, and the system does not crash (nor oopses). Greg, baring review turning up any issues, can you please queue this up for 3.18 ? Thanks & Regards, Hans *) Because the ASM1051 chokes on report opcodes -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/21] uas: Fix resetting flag handling
- Make sure we always hold the lock when setting / checking resetting
- Check resetting before checking urb->status
- Add missing check for resetting to uas_data_cmplt
- Add missing check for resetting to uas_do_work
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 49 +--
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 307658d..4f745a3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -127,6 +127,10 @@ static void uas_do_work(struct work_struct *work)
int err;
spin_lock_irqsave(&devinfo->lock, flags);
+
+ if (devinfo->resetting)
+ goto out;
+
list_for_each_entry(cmdinfo, &devinfo->inflight_list, list) {
struct scsi_pointer *scp = (void *)cmdinfo;
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
@@ -141,6 +145,7 @@ static void uas_do_work(struct work_struct *work)
else
schedule_work(&devinfo->work);
}
+out:
spin_unlock_irqrestore(&devinfo->lock, flags);
}
@@ -320,6 +325,11 @@ static void uas_stat_cmplt(struct urb *urb)
unsigned long flags;
u16 tag;
+ spin_lock_irqsave(&devinfo->lock, flags);
+
+ if (devinfo->resetting)
+ goto out;
+
if (urb->status) {
if (urb->status == -ENOENT) {
dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
@@ -328,27 +338,17 @@ static void uas_stat_cmplt(struct urb *urb)
dev_err(&urb->dev->dev, "stat urb: status %d\n",
urb->status);
}
- usb_free_urb(urb);
- return;
- }
-
- if (devinfo->resetting) {
- usb_free_urb(urb);
- return;
+ goto out;
}
- spin_lock_irqsave(&devinfo->lock, flags);
tag = be16_to_cpup(&iu->tag) - 1;
if (tag == 0)
cmnd = devinfo->cmnd;
else
cmnd = scsi_host_find_tag(shost, tag - 1);
- if (!cmnd) {
- usb_free_urb(urb);
- spin_unlock_irqrestore(&devinfo->lock, flags);
- return;
- }
+ if (!cmnd)
+ goto out;
cmdinfo = (void *)&cmnd->SCp;
switch (iu->iu_id) {
@@ -389,6 +389,7 @@ static void uas_stat_cmplt(struct urb *urb)
scmd_printk(KERN_ERR, cmnd,
"Bogus IU (%d) received on status pipe\n", iu->iu_id);
}
+out:
usb_free_urb(urb);
spin_unlock_irqrestore(&devinfo->lock, flags);
}
@@ -402,6 +403,7 @@ static void uas_data_cmplt(struct urb *urb)
unsigned long flags;
spin_lock_irqsave(&devinfo->lock, flags);
+
if (cmdinfo->data_in_urb == urb) {
sdb = scsi_in(cmnd);
cmdinfo->state &= ~DATA_IN_URB_INFLIGHT;
@@ -411,7 +413,13 @@ static void uas_data_cmplt(struct urb *urb)
}
if (sdb == NULL) {
WARN_ON_ONCE(1);
- } else if (urb->status) {
+ goto out;
+ }
+
+ if (devinfo->resetting)
+ goto out;
+
+ if (urb->status) {
if (urb->status != -ECONNRESET) {
uas_log_cmd_state(cmnd, __func__);
scmd_printk(KERN_ERR, cmnd,
@@ -424,6 +432,7 @@ static void uas_data_cmplt(struct urb *urb)
sdb->resid = sdb->length - urb->actual_length;
}
uas_try_complete(cmnd, __func__);
+out:
spin_unlock_irqrestore(&devinfo->lock, flags);
}
@@ -721,6 +730,7 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
struct scsi_device *sdev = cmnd->device;
struct uas_dev_info *devinfo = sdev->hostdata;
struct usb_device *udev = devinfo->udev;
+ unsigned long flags;
int err;
err = usb_lock_device_for_reset(udev, devinfo->intf);
@@ -731,14 +741,21 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd
*cmnd)
}
shost_printk(KERN_INFO, sdev->host, "%s start\n", __func__);
+
+ spin_lock_irqsave(&devinfo->lock, flags);
devinfo->resetting = 1;
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+
uas_abort_inflight(devinfo, DID_RESET, __func__);
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
uas_zap_dead(devinfo);
err = usb_reset_device(udev);
+
+ spin_lock_irqsave(&devinfo->lock, flags);
devinfo->resetting = 0;
+ spin_unlock_irqrestore(&devinfo->lock, flags);
usb_unlock_device(udev);
@@ -1032,8 +1049,12 @@ static void uas_disconnect(struct usb_interface *intf)
{
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_in
[PATCH 07/21] uas: Simplify unlink of data urbs on error
There is no need for all the trickery with dropping the lock, we can
simply reference the urbs while we hold the lock to ensure the urbs don't
disappear beneath us, and do the actual unlink (+ unreference) after we've
dropped the lock.
This also fixes a race where we may loose of cmnd ownership to the scsi
midlayer without holding the lock due to the midlayer re-claiming ownership
through an abort (which will be handled by a future patch in this series).
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 48 ++-
1 file changed, 18 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1ef59b0..b91b5e9 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -76,8 +76,7 @@ enum {
DATA_OUT_URB_INFLIGHT = (1 << 10),
COMMAND_COMPLETED = (1 << 11),
COMMAND_ABORTED = (1 << 12),
- UNLINK_DATA_URBS= (1 << 13),
- IS_IN_WORK_LIST = (1 << 14),
+ IS_IN_WORK_LIST = (1 << 13),
};
/* Overrides scsi_pointer */
@@ -98,28 +97,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller);
static void uas_free_streams(struct uas_dev_info *devinfo);
static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller);
-/* Must be called with devinfo->lock held, will temporary unlock the lock */
-static void uas_unlink_data_urbs(struct uas_dev_info *devinfo,
-struct uas_cmd_info *cmdinfo,
-unsigned long *lock_flags)
-{
- /*
-* The UNLINK_DATA_URBS flag makes sure uas_try_complete
-* (called by urb completion) doesn't release cmdinfo
-* underneath us.
-*/
- cmdinfo->state |= UNLINK_DATA_URBS;
- spin_unlock_irqrestore(&devinfo->lock, *lock_flags);
-
- if (cmdinfo->data_in_urb)
- usb_unlink_urb(cmdinfo->data_in_urb);
- if (cmdinfo->data_out_urb)
- usb_unlink_urb(cmdinfo->data_out_urb);
-
- spin_lock_irqsave(&devinfo->lock, *lock_flags);
- cmdinfo->state &= ~UNLINK_DATA_URBS;
-}
-
static void uas_do_work(struct work_struct *work)
{
struct uas_dev_info *devinfo =
@@ -279,8 +256,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const
char *caller)
{
struct uas_cmd_info *ci = (void *)&cmnd->SCp;
- scmd_printk(KERN_INFO, cmnd, "%s %p tag %d, inflight:"
- "%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+ scmd_printk(KERN_INFO, cmnd,
+ "%s %p tag %d, inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
caller, cmnd, uas_get_tag(cmnd),
(ci->state & SUBMIT_STATUS_URB) ? " s-st" : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "",
@@ -294,7 +271,6 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const
char *caller)
(ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT" : "",
(ci->state & COMMAND_COMPLETED) ? " done" : "",
(ci->state & COMMAND_ABORTED) ? " abort" : "",
- (ci->state & UNLINK_DATA_URBS) ? " unlink": "",
(ci->state & IS_IN_WORK_LIST) ? " work" : "");
}
@@ -306,8 +282,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
lockdep_assert_held(&devinfo->lock);
if (cmdinfo->state & (COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
- DATA_OUT_URB_INFLIGHT |
- UNLINK_DATA_URBS))
+ DATA_OUT_URB_INFLIGHT))
return -EBUSY;
WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
@@ -339,6 +314,8 @@ static void uas_stat_cmplt(struct urb *urb)
struct iu *iu = urb->transfer_buffer;
struct Scsi_Host *shost = urb->context;
struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
+ struct urb *data_in_urb = NULL;
+ struct urb *data_out_urb = NULL;
struct scsi_cmnd *cmnd;
struct uas_cmd_info *cmdinfo;
unsigned long flags;
@@ -385,7 +362,8 @@ static void uas_stat_cmplt(struct urb *urb)
uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
- uas_unlink_data_urbs(devinfo, cmdinfo, &flags);
+ data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+ data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
}
cmdinfo->state &= ~COMMAND_INFLIGHT;
uas_try_complete(cmnd, __func__);
@@ -413,6 +391,16 @@ static void uas_stat_cmplt(struct urb *urb)
out:
usb_free_urb(urb);
spin_unlock_irqrestore(&devinfo->lock, flags);
+
+ /* Unl
[PATCH 04/21] uas: Add uas_get_tag() helper function
Factor out the mapping of scsi-tags -> uas-tags/stream-ids to a helper function
so that there is a single place where this "magic" happens.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 33 +
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4f745a3..05c1c81 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -257,13 +257,29 @@ static void uas_sense_old(struct urb *urb, struct
scsi_cmnd *cmnd)
cmnd->result = sense_iu->status;
}
+/*
+ * scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
+ * which go from 1 - nr_streams. And we use 1 for untagged commands.
+ */
+static int uas_get_tag(struct scsi_cmnd *cmnd)
+{
+ int tag;
+
+ if (blk_rq_tagged(cmnd->request))
+ tag = cmnd->request->tag + 2;
+ else
+ tag = 1;
+
+ return tag;
+}
+
static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller)
{
struct uas_cmd_info *ci = (void *)&cmnd->SCp;
scmd_printk(KERN_INFO, cmnd, "%s %p tag %d, inflight:"
"%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
- caller, cmnd, cmnd->request->tag,
+ caller, cmnd, uas_get_tag(cmnd),
(ci->state & SUBMIT_STATUS_URB) ? " s-st" : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "",
(ci->state & SUBMIT_DATA_IN_URB)? " s-in" : "",
@@ -514,10 +530,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
goto free;
iu->iu_id = IU_ID_COMMAND;
- if (blk_rq_tagged(cmnd->request))
- iu->tag = cpu_to_be16(cmnd->request->tag + 2);
- else
- iu->tag = cpu_to_be16(1);
+ iu->tag = cpu_to_be16(uas_get_tag(cmnd));
iu->prio_attr = UAS_SIMPLE_TAG;
iu->len = len;
int_to_scsilun(sdev->lun, &iu->lun);
@@ -679,17 +692,13 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
memset(cmdinfo, 0, sizeof(*cmdinfo));
- if (blk_rq_tagged(cmnd->request)) {
- cmdinfo->stream = cmnd->request->tag + 2;
- } else {
+ if (!blk_rq_tagged(cmnd->request))
devinfo->cmnd = cmnd;
- cmdinfo->stream = 1;
- }
cmnd->scsi_done = done;
- cmdinfo->state = SUBMIT_STATUS_URB |
- ALLOC_CMD_URB | SUBMIT_CMD_URB;
+ cmdinfo->stream = uas_get_tag(cmnd);
+ cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
switch (cmnd->sc_data_direction) {
case DMA_FROM_DEVICE:
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/21] uas: Do not log urb status error on cancellation
Check for both type of cancellation codes for sense and data urbs.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 685eb37..46b8788 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -315,10 +315,7 @@ static void uas_stat_cmplt(struct urb *urb)
goto out;
if (urb->status) {
- if (urb->status == -ENOENT) {
- dev_err(&urb->dev->dev, "stat urb: killed, stream %d\n",
- urb->stream_id);
- } else {
+ if (urb->status != -ENOENT && urb->status != -ECONNRESET) {
dev_err(&urb->dev->dev, "stat urb: status %d\n",
urb->status);
}
@@ -425,7 +422,7 @@ static void uas_data_cmplt(struct urb *urb)
}
if (urb->status) {
- if (urb->status != -ECONNRESET) {
+ if (urb->status != -ENOENT && urb->status != -ECONNRESET) {
uas_log_cmd_state(cmnd, __func__);
scmd_printk(KERN_ERR, cmnd,
"data cmplt err %d stream %d\n",
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/21] uas: Remove cmnd reference from the cmd urb
It is not strictly necessary for the cmd urb to have a reference to the
cmnd, and without this reference it becomes easier to drop all references to
a cmnd on an abort.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index dca09de..1fdcfad 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -425,12 +425,9 @@ out:
static void uas_cmd_cmplt(struct urb *urb)
{
- struct scsi_cmnd *cmnd = urb->context;
+ if (urb->status)
+ dev_err(&urb->dev->dev, "cmd cmplt err %d\n", urb->status);
- if (urb->status) {
- uas_log_cmd_state(cmnd, __func__);
- scmd_printk(KERN_ERR, cmnd, "cmd cmplt err %d\n", urb->status);
- }
usb_free_urb(urb);
}
@@ -508,7 +505,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info
*devinfo, gfp_t gfp,
memcpy(iu->cdb, cmnd->cmnd, cmnd->cmd_len);
usb_fill_bulk_urb(urb, udev, devinfo->cmd_pipe, iu, sizeof(*iu) + len,
- uas_cmd_cmplt, cmnd);
+ uas_cmd_cmplt, NULL);
urb->transfer_flags |= URB_FREE_BUFFER;
out:
return urb;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/21] uas: pre_reset and suspend: Fix a few races
The purpose of uas_pre_reset is to:
1) Stop any new commands from being submitted while an externally triggered
usb-device-reset is running
2) Wait for any pending commands to finish before allowing the usb-device-reset
to continue
The purpose of uas_suspend is to:
2) Wait for any pending commands to finish before suspending
This commit fixes races in both paths:
1) For 1) we use scsi_block_requests, but the scsi midlayer calls queuecommand
without holding any locks, so a queuecommand may already past the midlayer
scsi_block_requests checks when we call it, add a check to uas_queuecommand
to fix this
2) For 2) we were waiting for all sense-urbs to complete, there are 2 problems
with this approach:
a) data-urbs may complete after the sense urb, so we need to check for those
too
b) if a sense-urb completes with a iu id of READ/WRITE_READY a command is not
yet done. We submit a new sense-urb immediately in this case, but that
submit may fail (in which case it will get retried by uas_do_work), if this
happens the sense_urbs anchor may become empty while the cmnd is not yet
done
Also unblock requests on timeout, to avoid things getting stuck in that case.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 61 ++-
1 file changed, 55 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6ee61bd..aaca65b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -664,6 +664,10 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
+ /* Re-check scsi_block_requests now that we've the host-lock */
+ if (cmnd->device->host->host_self_blocked)
+ return SCSI_MLQUEUE_DEVICE_BUSY;
+
spin_lock_irqsave(&devinfo->lock, flags);
if (devinfo->resetting) {
@@ -991,6 +995,54 @@ set_alt0:
return result;
}
+static int uas_cmnd_list_empty(struct uas_dev_info *devinfo)
+{
+ unsigned long flags;
+ int i, r = 1;
+
+ spin_lock_irqsave(&devinfo->lock, flags);
+
+ for (i = 0; i < devinfo->qdepth; i++) {
+ if (devinfo->cmnd[i]) {
+ r = 0; /* Not empty */
+ break;
+ }
+ }
+
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+
+ return r;
+}
+
+/*
+ * Wait for any pending cmnds to complete, on usb-2 sense_urbs may temporarily
+ * get empty while there still is more work to do due to sense-urbs completing
+ * with a READ/WRITE_READY iu code, so keep waiting until the list gets empty.
+ */
+static int uas_wait_for_pending_cmnds(struct uas_dev_info *devinfo)
+{
+ unsigned long start_time;
+ int r;
+
+ start_time = jiffies;
+ do {
+ flush_work(&devinfo->work);
+
+ r = usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000);
+ if (r == 0)
+ return -ETIME;
+
+ r = usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500);
+ if (r == 0)
+ return -ETIME;
+
+ if (time_after(jiffies, start_time + 5 * HZ))
+ return -ETIME;
+ } while (!uas_cmnd_list_empty(devinfo));
+
+ return 0;
+}
+
static int uas_pre_reset(struct usb_interface *intf)
{
struct Scsi_Host *shost = usb_get_intfdata(intf);
@@ -1005,10 +1057,9 @@ static int uas_pre_reset(struct usb_interface *intf)
scsi_block_requests(shost);
spin_unlock_irqrestore(shost->host_lock, flags);
- /* Wait for any pending requests to complete */
- flush_work(&devinfo->work);
- if (usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000) == 0) {
+ if (uas_wait_for_pending_cmnds(devinfo) != 0) {
shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
+ scsi_unblock_requests(shost);
return 1;
}
@@ -1046,9 +1097,7 @@ static int uas_suspend(struct usb_interface *intf,
pm_message_t message)
struct Scsi_Host *shost = usb_get_intfdata(intf);
struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
- /* Wait for any pending requests to complete */
- flush_work(&devinfo->work);
- if (usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000) == 0) {
+ if (uas_wait_for_pending_cmnds(devinfo) != 0) {
shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
return -ETIME;
}
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/21] uas: Simplify reset / disconnect handling
Drop the whole dance with first moving cmnds to a dead-list. The resetting
flag ensures that no new cmds / urbs will be submitted, and that any urb
completions are short-circuited without trying to complete the scsi cmnd.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 43 ++-
1 file changed, 6 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4c7d337..bba5498 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -60,7 +60,6 @@ struct uas_dev_info {
spinlock_t lock;
struct work_struct work;
struct list_head inflight_list;
- struct list_head dead_list;
};
enum {
@@ -128,35 +127,6 @@ out:
spin_unlock_irqrestore(&devinfo->lock, flags);
}
-static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
- struct uas_cmd_info *cmdinfo,
- int result, const char *caller)
-{
- struct scsi_pointer *scp = (void *)cmdinfo;
- struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
-
- uas_log_cmd_state(cmnd, caller);
- lockdep_assert_held(&devinfo->lock);
- WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
- cmdinfo->state |= COMMAND_ABORTED;
- cmdinfo->state &= ~IS_IN_WORK_LIST;
- cmnd->result = result << 16;
- list_move_tail(&cmdinfo->list, &devinfo->dead_list);
-}
-
-static void uas_abort_inflight(struct uas_dev_info *devinfo, int result,
- const char *caller)
-{
- struct uas_cmd_info *cmdinfo;
- struct uas_cmd_info *temp;
- unsigned long flags;
-
- spin_lock_irqsave(&devinfo->lock, flags);
- list_for_each_entry_safe(cmdinfo, temp, &devinfo->inflight_list, list)
- uas_mark_cmd_dead(devinfo, cmdinfo, result, caller);
- spin_unlock_irqrestore(&devinfo->lock, flags);
-}
-
static void uas_add_work(struct uas_cmd_info *cmdinfo)
{
struct scsi_pointer *scp = (void *)cmdinfo;
@@ -168,7 +138,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
schedule_work(&devinfo->work);
}
-static void uas_zap_dead(struct uas_dev_info *devinfo)
+static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
{
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
@@ -180,11 +150,11 @@ static void uas_zap_dead(struct uas_dev_info *devinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
uas_log_cmd_state(cmnd, __func__);
- WARN_ON_ONCE(!(cmdinfo->state & COMMAND_ABORTED));
/* all urbs are killed, clear inflight bits */
cmdinfo->state &= ~(COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
DATA_OUT_URB_INFLIGHT);
+ cmnd->result = result << 16;
uas_try_complete(cmnd, __func__);
}
spin_unlock_irqrestore(&devinfo->lock, flags);
@@ -754,11 +724,11 @@ static int uas_eh_bus_reset_handler(struct scsi_cmnd
*cmnd)
devinfo->resetting = 1;
spin_unlock_irqrestore(&devinfo->lock, flags);
- uas_abort_inflight(devinfo, DID_RESET, __func__);
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
- uas_zap_dead(devinfo);
+ uas_zap_pending(devinfo, DID_RESET);
+
err = usb_reset_device(udev);
spin_lock_irqsave(&devinfo->lock, flags);
@@ -935,7 +905,6 @@ static int uas_probe(struct usb_interface *intf, const
struct usb_device_id *id)
spin_lock_init(&devinfo->lock);
INIT_WORK(&devinfo->work, uas_do_work);
INIT_LIST_HEAD(&devinfo->inflight_list);
- INIT_LIST_HEAD(&devinfo->dead_list);
result = uas_configure_endpoints(devinfo);
if (result)
@@ -1063,11 +1032,11 @@ static void uas_disconnect(struct usb_interface *intf)
spin_unlock_irqrestore(&devinfo->lock, flags);
cancel_work_sync(&devinfo->work);
- uas_abort_inflight(devinfo, DID_NO_CONNECT, __func__);
usb_kill_anchored_urbs(&devinfo->cmd_urbs);
usb_kill_anchored_urbs(&devinfo->sense_urbs);
usb_kill_anchored_urbs(&devinfo->data_urbs);
- uas_zap_dead(devinfo);
+ uas_zap_pending(devinfo, DID_NO_CONNECT);
+
scsi_remove_host(shost);
uas_free_streams(devinfo);
scsi_host_put(shost);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/21] uas: Drop all references to a scsi_cmnd once it has been aborted
Do not keep references around to a cmnd which is under error handling.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 47 ---
1 file changed, 44 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 1fdcfad..75ce40c 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -254,12 +254,11 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
lockdep_assert_held(&devinfo->lock);
if (cmdinfo->state & (COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
- DATA_OUT_URB_INFLIGHT))
+ DATA_OUT_URB_INFLIGHT |
+ COMMAND_ABORTED))
return -EBUSY;
WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
- if (cmdinfo->state & COMMAND_ABORTED)
- scmd_printk(KERN_INFO, cmnd, "abort completed\n");
devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
cmnd->scsi_done(cmnd);
return 0;
@@ -700,6 +699,47 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
static DEF_SCSI_QCMD(uas_queuecommand)
+/*
+ * For now we do not support actually sending an abort to the device, so
+ * this eh always fails. Still we must define it to make sure that we've
+ * dropped all references to the cmnd in question once this function exits.
+ */
+static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
+{
+ struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
+ struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+ struct urb *data_in_urb = NULL;
+ struct urb *data_out_urb = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(&devinfo->lock, flags);
+
+ uas_log_cmd_state(cmnd, __func__);
+
+ /* Ensure that try_complete does not call scsi_done */
+ cmdinfo->state |= COMMAND_ABORTED;
+
+ /* Drop all refs to this cmnd, kill data urbs to break their ref */
+ devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
+ if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+ data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+ if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+ data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+
+ spin_unlock_irqrestore(&devinfo->lock, flags);
+
+ if (data_in_urb) {
+ usb_kill_urb(data_in_urb);
+ usb_put_urb(data_in_urb);
+ }
+ if (data_out_urb) {
+ usb_kill_urb(data_out_urb);
+ usb_put_urb(data_out_urb);
+ }
+
+ return FAILED;
+}
+
static int uas_eh_bus_reset_handler(struct scsi_cmnd *cmnd)
{
struct scsi_device *sdev = cmnd->device;
@@ -781,6 +821,7 @@ static struct scsi_host_template uas_host_template = {
.queuecommand = uas_queuecommand,
.slave_alloc = uas_slave_alloc,
.slave_configure = uas_slave_configure,
+ .eh_abort_handler = uas_eh_abort_handler,
.eh_bus_reset_handler = uas_eh_bus_reset_handler,
.can_queue = 65536, /* Is there a limit on the _host_ ? */
.this_id = -1,
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/21] uas: Check against unexpected completions
The status urb should not complete before the command has been submitted, nor
should we get a second status urb for the same tag after a IU_ID_STATUS.
Data urbs should not complete before the command has been submitted, but may
complete after the IU_ID_STATUS.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index b2423d3..1ef59b0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -369,6 +369,12 @@ static void uas_stat_cmplt(struct urb *urb)
cmnd = devinfo->cmnd[idx];
cmdinfo = (void *)&cmnd->SCp;
+
+ if (!(cmdinfo->state & COMMAND_INFLIGHT)) {
+ scmd_printk(KERN_ERR, cmnd, "unexpected status cmplt\n");
+ goto out;
+ }
+
switch (iu->iu_id) {
case IU_ID_STATUS:
if (urb->actual_length < 16)
@@ -434,6 +440,12 @@ static void uas_data_cmplt(struct urb *urb)
if (devinfo->resetting)
goto out;
+ /* Data urbs should not complete before the cmd urb is submitted */
+ if (cmdinfo->state & SUBMIT_CMD_URB) {
+ scmd_printk(KERN_ERR, cmnd, "unexpected data cmplt\n");
+ goto out;
+ }
+
if (urb->status) {
if (urb->status != -ECONNRESET) {
uas_log_cmd_state(cmnd, __func__);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/21] uas: Fix memleak of non-submitted urbs
Not all urbs we've allocated are necessarily also submitted, non-submitted
urbs will not be free-ed by their completion handler. So we need to free
them manually.
There are 2 scenarios where this can happen:
1) We have failed to submit some urbs at abort / disconnect
2) When running over usb-2 we may have never tried to submit the data urbs
when completing the scsi cmnd, because we never got a READ/WRITE_READY iu
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 75ce40c..6ee61bd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -246,6 +246,25 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd,
const char *caller)
(ci->state & IS_IN_WORK_LIST) ? " work" : "");
}
+static void uas_free_unsubmitted_urbs(struct scsi_cmnd *cmnd)
+{
+ struct uas_cmd_info *cmdinfo;
+
+ if (!cmnd)
+ return;
+
+ cmdinfo = (void *)&cmnd->SCp;
+
+ if (cmdinfo->state & SUBMIT_CMD_URB)
+ usb_free_urb(cmdinfo->cmd_urb);
+
+ /* data urbs may have never gotten their submit flag set */
+ if (!(cmdinfo->state & DATA_IN_URB_INFLIGHT))
+ usb_free_urb(cmdinfo->data_in_urb);
+ if (!(cmdinfo->state & DATA_OUT_URB_INFLIGHT))
+ usb_free_urb(cmdinfo->data_out_urb);
+}
+
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
{
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
@@ -260,6 +279,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
cmdinfo->state |= COMMAND_COMPLETED;
devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
+ uas_free_unsubmitted_urbs(cmnd);
cmnd->scsi_done(cmnd);
return 0;
}
@@ -726,6 +746,8 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+ uas_free_unsubmitted_urbs(cmnd);
+
spin_unlock_irqrestore(&devinfo->lock, flags);
if (data_in_urb) {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 21/21] uas: Remove protype hardware usb interface info
We've removed all hack from the driver for pre-production hardware.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 23c0b97..a7f16c4 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -818,8 +818,6 @@ static struct usb_device_id uas_usb_ids[] = {
# include "unusual_uas.h"
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_BULK)
},
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_UAS) },
- /* 0xaa is a prototype device I happen to have access to */
- { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, 0xaa) },
{ }
};
MODULE_DEVICE_TABLE(usb, uas_usb_ids);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 20/21] uas: Remove support for old sense ui as used in pre-production hardware
I've access to a number of different uas devices now, and none of them use
old style sense urbs. The only case where these code-paths trigger is with
the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
status iu-s when it does not have any sense data, but uses new style
sense iu-s regardless, as can be seen for scsi cmnds where there is sense
data.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 47 +--
1 file changed, 1 insertion(+), 46 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index e8d2bf1..23c0b97 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -31,20 +31,6 @@
#define MAX_CMNDS 256
-/*
- * The r00-r01c specs define this version of the SENSE IU data structure.
- * It's still in use by several different firmware releases.
- */
-struct sense_iu_old {
- __u8 iu_id;
- __u8 rsvd1;
- __be16 tag;
- __be16 len;
- __u8 status;
- __u8 service_response;
- __u8 sense[SCSI_SENSE_BUFFERSIZE];
-};
-
struct uas_dev_info {
struct usb_interface *intf;
struct usb_device *udev;
@@ -54,7 +40,6 @@ struct uas_dev_info {
int qdepth, resetting;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
unsigned use_streams:1;
- unsigned uas_sense_old:1;
unsigned shutdown:1;
struct scsi_cmnd *cmnd[MAX_CMNDS];
spinlock_t lock;
@@ -184,29 +169,6 @@ static void uas_sense(struct urb *urb, struct scsi_cmnd
*cmnd)
cmnd->result = sense_iu->status;
}
-static void uas_sense_old(struct urb *urb, struct scsi_cmnd *cmnd)
-{
- struct sense_iu_old *sense_iu = urb->transfer_buffer;
- struct scsi_device *sdev = cmnd->device;
-
- if (urb->actual_length > 8) {
- unsigned len = be16_to_cpup(&sense_iu->len) - 2;
- if (len + 8 != urb->actual_length) {
- int newlen = min(len + 8, urb->actual_length) - 8;
- if (newlen < 0)
- newlen = 0;
- sdev_printk(KERN_INFO, sdev, "%s: urb length %d "
- "disagrees with IU sense data length %d, "
- "using %d bytes of sense data\n", __func__,
- urb->actual_length, len, newlen);
- len = newlen;
- }
- memcpy(cmnd->sense_buffer, sense_iu->sense, len);
- }
-
- cmnd->result = sense_iu->status;
-}
-
/*
* scsi-tags go from 0 - (nr_tags - 1), uas tags need to match stream-ids,
* which go from 1 - nr_streams. And we use 1 for untagged commands.
@@ -336,12 +298,7 @@ static void uas_stat_cmplt(struct urb *urb)
switch (iu->iu_id) {
case IU_ID_STATUS:
- if (urb->actual_length < 16)
- devinfo->uas_sense_old = 1;
- if (devinfo->uas_sense_old)
- uas_sense_old(urb, cmnd);
- else
- uas_sense(urb, cmnd);
+ uas_sense(urb, cmnd);
if (cmnd->result != 0) {
/* cancel data transfers on error */
data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
@@ -888,8 +845,6 @@ static int uas_configure_endpoints(struct uas_dev_info
*devinfo)
struct usb_device *udev = devinfo->udev;
int r;
- devinfo->uas_sense_old = 0;
-
r = uas_find_endpoints(devinfo->intf->cur_altsetting, eps);
if (r)
return r;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/21] uas: Drop inflight list
We've the same info doubled in both the inflight list and the cmnd array,
drop the list.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index ec6547d..dca09de 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -59,7 +59,6 @@ struct uas_dev_info {
struct scsi_cmnd *cmnd[MAX_CMNDS];
spinlock_t lock;
struct work_struct work;
- struct list_head inflight_list;
};
enum {
@@ -85,7 +84,6 @@ struct uas_cmd_info {
struct urb *cmd_urb;
struct urb *data_in_urb;
struct urb *data_out_urb;
- struct list_head list;
};
/* I hate forward declarations, but I actually have a loop */
@@ -101,18 +99,21 @@ static void uas_do_work(struct work_struct *work)
struct uas_dev_info *devinfo =
container_of(work, struct uas_dev_info, work);
struct uas_cmd_info *cmdinfo;
+ struct scsi_cmnd *cmnd;
unsigned long flags;
- int err;
+ int i, err;
spin_lock_irqsave(&devinfo->lock, flags);
if (devinfo->resetting)
goto out;
- list_for_each_entry(cmdinfo, &devinfo->inflight_list, list) {
- struct scsi_pointer *scp = (void *)cmdinfo;
- struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
- SCp);
+ for (i = 0; i < devinfo->qdepth; i++) {
+ if (!devinfo->cmnd[i])
+ continue;
+
+ cmnd = devinfo->cmnd[i];
+ cmdinfo = (void *)&cmnd->SCp;
if (!(cmdinfo->state & IS_IN_WORK_LIST))
continue;
@@ -141,14 +142,17 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
{
struct uas_cmd_info *cmdinfo;
- struct uas_cmd_info *temp;
+ struct scsi_cmnd *cmnd;
unsigned long flags;
+ int i;
spin_lock_irqsave(&devinfo->lock, flags);
- list_for_each_entry_safe(cmdinfo, temp, &devinfo->dead_list, list) {
- struct scsi_pointer *scp = (void *)cmdinfo;
- struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
- SCp);
+ for (i = 0; i < devinfo->qdepth; i++) {
+ if (!devinfo->cmnd[i])
+ continue;
+
+ cmnd = devinfo->cmnd[i];
+ cmdinfo = (void *)&cmnd->SCp;
uas_log_cmd_state(cmnd, __func__);
/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
cmdinfo->state &= ~COMMAND_INFLIGHT;
@@ -256,7 +260,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
cmdinfo->state |= COMMAND_COMPLETED;
if (cmdinfo->state & COMMAND_ABORTED)
scmd_printk(KERN_INFO, cmnd, "abort completed\n");
- list_del(&cmdinfo->list);
devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
cmnd->scsi_done(cmnd);
return 0;
@@ -694,7 +697,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
}
devinfo->cmnd[stream - 1] = cmnd;
- list_add_tail(&cmdinfo->list, &devinfo->inflight_list);
spin_unlock_irqrestore(&devinfo->lock, flags);
return 0;
}
@@ -902,7 +904,6 @@ static int uas_probe(struct usb_interface *intf, const
struct usb_device_id *id)
init_usb_anchor(&devinfo->data_urbs);
spin_lock_init(&devinfo->lock);
INIT_WORK(&devinfo->work, uas_do_work);
- INIT_LIST_HEAD(&devinfo->inflight_list);
result = uas_configure_endpoints(devinfo);
if (result)
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 19/21] uas: Drop COMMAND_COMPLETED flag
It was only used to sanity check against completing the same cmnd twice,
but that is the case we're likely operating on free-ed memory, and doing
sanity checks on free-ed memory is not really helpful.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 220f4c7..e8d2bf1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -72,9 +72,8 @@ enum {
COMMAND_INFLIGHT= (1 << 8),
DATA_IN_URB_INFLIGHT= (1 << 9),
DATA_OUT_URB_INFLIGHT = (1 << 10),
- COMMAND_COMPLETED = (1 << 11),
- COMMAND_ABORTED = (1 << 12),
- IS_IN_WORK_LIST = (1 << 13),
+ COMMAND_ABORTED = (1 << 11),
+ IS_IN_WORK_LIST = (1 << 12),
};
/* Overrides scsi_pointer */
@@ -229,7 +228,7 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const
char *caller)
struct uas_cmd_info *ci = (void *)&cmnd->SCp;
scmd_printk(KERN_INFO, cmnd,
- "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ",
+ "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s ",
caller, uas_get_tag(cmnd),
(ci->state & SUBMIT_STATUS_URB) ? " s-st" : "",
(ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "",
@@ -241,7 +240,6 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const
char *caller)
(ci->state & COMMAND_INFLIGHT) ? " CMD" : "",
(ci->state & DATA_IN_URB_INFLIGHT) ? " IN": "",
(ci->state & DATA_OUT_URB_INFLIGHT) ? " OUT" : "",
- (ci->state & COMMAND_COMPLETED) ? " done" : "",
(ci->state & COMMAND_ABORTED) ? " abort" : "",
(ci->state & IS_IN_WORK_LIST) ? " work" : "");
scsi_print_command(cmnd);
@@ -277,8 +275,6 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
DATA_OUT_URB_INFLIGHT |
COMMAND_ABORTED))
return -EBUSY;
- WARN_ON_ONCE(cmdinfo->state & COMMAND_COMPLETED);
- cmdinfo->state |= COMMAND_COMPLETED;
devinfo->cmnd[uas_get_tag(cmnd) - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
cmnd->scsi_done(cmnd);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/21] uas: Use scsi_print_command
Use scsi_print_command to print commands during errors, rather then printing the rather meaningless pointer to the command. Signed-off-by: Hans de Goede --- drivers/usb/storage/uas.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 46b8788..220f4c7 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -229,8 +229,8 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller) struct uas_cmd_info *ci = (void *)&cmnd->SCp; scmd_printk(KERN_INFO, cmnd, - "%s %p tag %d, inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s\n", - caller, cmnd, uas_get_tag(cmnd), + "%s tag %d inflight:%s%s%s%s%s%s%s%s%s%s%s%s%s ", + caller, uas_get_tag(cmnd), (ci->state & SUBMIT_STATUS_URB) ? " s-st" : "", (ci->state & ALLOC_DATA_IN_URB) ? " a-in" : "", (ci->state & SUBMIT_DATA_IN_URB)? " s-in" : "", @@ -244,6 +244,7 @@ static void uas_log_cmd_state(struct scsi_cmnd *cmnd, const char *caller) (ci->state & COMMAND_COMPLETED) ? " done" : "", (ci->state & COMMAND_ABORTED) ? " abort" : "", (ci->state & IS_IN_WORK_LIST) ? " work" : ""); + scsi_print_command(cmnd); } static void uas_free_unsubmitted_urbs(struct scsi_cmnd *cmnd) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/21] uas: Use streams on upcoming 10Gbps / 3.1 USB
Limit the no-streams case to speeds less then USB_SPEED_SUPER.
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index aaca65b..685eb37 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -909,7 +909,7 @@ static int uas_configure_endpoints(struct uas_dev_info
*devinfo)
devinfo->data_out_pipe = usb_sndbulkpipe(udev,
usb_endpoint_num(&eps[3]->desc));
- if (udev->speed != USB_SPEED_SUPER) {
+ if (udev->speed < USB_SPEED_SUPER) {
devinfo->qdepth = 32;
devinfo->use_streams = 0;
} else {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
From: Sanjeev Sharma
On some architecture spin_is_locked() always return false in
uniprocessor configuration and therefore it would be advise to replace
with lockdep_assert_held().
Signed-off-by: Sanjeev Sharma
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..05b2d8e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
uas_log_cmd_state(cmnd, caller);
- WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+ lockdep_assert_held(&devinfo->lock);
WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
cmdinfo->state |= COMMAND_ABORTED;
cmdinfo->state &= ~IS_IN_WORK_LIST;
@@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
struct uas_dev_info *devinfo = cmnd->device->hostdata;
- WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+ lockdep_assert_held(&devinfo->lock);
cmdinfo->state |= IS_IN_WORK_LIST;
schedule_work(&devinfo->work);
}
@@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
char *caller)
struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
- WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+ lockdep_assert_held(&devinfo->lock);
if (cmdinfo->state & (COMMAND_INFLIGHT |
DATA_IN_URB_INFLIGHT |
DATA_OUT_URB_INFLIGHT |
@@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
struct urb *urb;
int err;
- WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+ lockdep_assert_held(&devinfo->lock);
if (cmdinfo->state & SUBMIT_STATUS_URB) {
urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
if (!urb)
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead
On 09/10/2014 12:15 PM, Kashyap Desai wrote:
>> -Original Message-
>> From: Tomas Henzl [mailto:the...@redhat.com]
>> Sent: Tuesday, September 09, 2014 7:01 PM
>> To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
>> Cc: martin.peter...@oracle.com; h...@infradead.org;
>> jbottom...@parallels.com; kashyap.de...@avagotech.com;
>> aradf...@gmail.com
>> Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write
> to
>> avoid spinlock overhead
>>
>> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>>> Use writeq() for 64bit PCI write instead of writel() to avoid
> additional lock
>> overhead.
>>> Signed-off-by: Sumit Saxena
>>> Signed-off-by: Kashyap Desai
>>> ---
>>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 8
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> index 57b47fe..c69c1ac 100644
>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> @@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct
>> megasas_instance *instance,
>>> u32 req_desc_hi,
>>> struct megasas_register_set __iomem *regs)
>> Hi Sumit,
>> the fn params are a bit confusing req_desc_lo is of type dma_addr_t and
>> req_desc_hi is u32, is it possible to unite it in the future?
> Agree. We should make changes here. We will do it in separate patch.
> Originally fire_cmd() was written for MFI controller and carry forward for
> all generation.
> In MFI it use second argument as 32 bit address and third argument as
> frame count, but later in Fusion adapter it started using differently.
ok
>
>>> {
>>> +#if defined(writeq) && defined(CONFIG_64BIT)
>> On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq"
>> if it's incorrect fix it there too or remove the CONFIG_64 here
> We would like to change at mpt2sas as we have all the code with below
> check for writeq()
> Original discuss was started when we submitted this change in mpt2sas, but
> we have delta from day-1.
> LSI/Avago internal source has "#if defined(writeq) &&
> defined(CONFIG_64BIT)" check in mpt2sas.
>
> I think now writeq() is implemented in all arch, so we can safely remove
> check for #if writeq().
> But we can keep this check as it is to continue for older Distribution to
> take direct advantage without maintaining any separate patch.
I don't know which combination of writeq and config_64bit is
the right way, I was hoping that someone who knows will help with.
(I'll accept almost any combination you'll post...)
>
>>> + u64 req_data = 0;
>>> +
>>> + req_data = req_desc_hi;
>>> + req_data = ((req_data << 32) | (u32)req_desc_lo);
>> This seems to be critical path (you are removing an spinlock to avoid
>> overhead), so why do you have three consecutive assignments to the same
>> variable?
>> (~(u64 req_data = r_hi << 32 | r_lo))
>
> Agree. We will be doing this change and re-submit the patch to address
> this.
Thanks.
>
>> Cheers,
>> Tomas
>>
>>> + writeq(le64_to_cpu(req_data), &(regs)-
>>> inbound_low_queue_port);
>>> +#else
>>> unsigned long flags;
>>>
>>> spin_lock_irqsave(&instance->hba_lock, flags); @@ -1072,6 +1079,7
>> @@
>>> megasas_fire_cmd_fusion(struct megasas_instance *instance,
>>> writel(le32_to_cpu(req_desc_lo), &(regs)-
>>> inbound_low_queue_port);
>>> writel(le32_to_cpu(req_desc_hi), &(regs)-
>>> inbound_high_queue_port);
>>> spin_unlock_irqrestore(&instance->hba_lock, flags);
>>> +#endif
>>> }
>>>
>>> /**
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 03/11] megaraid_sas : Update threshold based reply post host index register
>-Original Message-
>From: Tomas Henzl [mailto:the...@redhat.com]
>Sent: Tuesday, September 09, 2014 7:39 PM
>To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
>Cc: martin.peter...@oracle.com; h...@infradead.org;
>jbottom...@parallels.com; kashyap.de...@avagotech.com;
>aradf...@gmail.com
>Subject: Re: [PATCH 03/11] megaraid_sas : Update threshold based reply
post
>host index register
>
>On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>> Current driver updates reply post host index to let firmware know that
>> replies are processed, while returning from ISR function, only if there
is no
>oustanding replies in reply queue.
>>
>> Driver will free the request frame immediately from ISR but reply post
host
>index is not yet updated.
>> It means freed request can be used by submission path and there may be
>> a tight loop in request/reply path. In such condition, firmware may
>> crash when it tries to post reply and there is no free reply post
descriptor.
>>
>> Eventually two things needs to be change to avoid this issue.
>>
>> Increase reply queue depth (double than request queue) to accommodate
>worst case scenario.
>> Update reply post host index to firmware once it reach to some
pre-defined
>threshold value.
>>
>> This change will make sure that firmware will always have some buffer
>> of reply descriptor and will never find empty reply descriptor in
completion
>path.
>>
>> Signed-off-by: Sumit Saxena
>> Signed-off-by: Kashyap Desai
>> ---
>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 23
>> ++-
>drivers/scsi/megaraid/megaraid_sas_fusion.h |
>> 1 +
>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> index c69c1ac..f30297d 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> @@ -971,7 +971,7 @@ megasas_init_adapter_fusion(struct
>> megasas_instance *instance)
>>
>> max_cmd = instance->max_fw_cmds;
>>
>> -fusion->reply_q_depth = ((max_cmd + 1 + 15)/16)*16;
>> +fusion->reply_q_depth = (((max_cmd * 2) + 1 + 15)/16)*16;
>
>This computation gives for certain values the same result as before, for
some
>the result is 1.5*higher and for others 2*higher. Is this intended?
>(I'm not asking what all the magic numbers mean..) what about a simple
>fusion->reply_q_depth = 2*(((max_cmd + 1 + 15)/16)*16);
I agree to make this computation simple and more accurate to give always
2*higher.
In most of the cases, we need reply queue depth = request_queue_depth +
additional room as provide in THRESHOLD_REPLY_COUNT.
Thanks for the feedback. We will do this change and will resubmit the
patch.
>
>>
>> fusion->request_alloc_sz =
>> sizeof(union MEGASAS_REQUEST_DESCRIPTOR_UNION)
>*max_cmd; @@ -1876,6
>> +1876,7 @@ complete_cmd_fusion(struct megasas_instance *instance, u32
>MSIxIndex)
>> u32 status, extStatus, device_id;
>> union desc_value d_val;
>> struct LD_LOAD_BALANCE_INFO *lbinfo;
>> +int threshold_reply_count = 0;
>>
>> fusion = instance->ctrl_context;
>>
>> @@ -1963,6 +1964,7 @@ complete_cmd_fusion(struct megasas_instance
>> *instance, u32 MSIxIndex)
>>
>> desc->Words = ULLONG_MAX;
>> num_completed++;
>> +threshold_reply_count++;
>>
>> /* Get the next reply descriptor */
>> if (!fusion->last_reply_idx[MSIxIndex])
>> @@ -1982,6 +1984,25 @@ complete_cmd_fusion(struct megasas_instance
>> *instance, u32 MSIxIndex)
>>
>> if (reply_descript_type ==
>MPI2_RPY_DESCRIPT_FLAGS_UNUSED)
>> break;
>> +/*
>> + * Write to reply post host index register after
completing
>threshold
>> + * number of reply counts and still there are more replies
in
>reply queue
>> + * pending to be completed
>> + */
>> +if (threshold_reply_count >= THRESHOLD_REPLY_COUNT) {
>> +if ((instance->pdev->device ==
>> +PCI_DEVICE_ID_LSI_INVADER) ||
>> +(instance->pdev->device ==
>> +PCI_DEVICE_ID_LSI_FURY))
>> +writel(((MSIxIndex & 0x7) << 24) |
>> +fusion->last_reply_idx[MSIxIndex],
>> +instance-
>>reply_post_host_index_addr[MSIxIndex/8]);
>> +else
>> +writel((MSIxIndex << 24) |
>> +fusion->last_reply_idx[MSIxIndex],
>> +instance-
>>reply_post_host_index_addr[0]);
>> +threshold_reply_count = 0;
>> +}
>> }
>>
>> if (!num_completed)
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.h
>> b/drivers/scsi/megaraid/megaraid_sas_fusion.h
>> index e76af54..d660c
RE: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write to avoid spinlock overhead
> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Tuesday, September 09, 2014 7:01 PM
> To: sumit.sax...@avagotech.com; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; h...@infradead.org;
> jbottom...@parallels.com; kashyap.de...@avagotech.com;
> aradf...@gmail.com
> Subject: Re: [PATCH 02/11] megaraid_sas : Use writeq for 64bit pci write
to
> avoid spinlock overhead
>
> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
> > Use writeq() for 64bit PCI write instead of writel() to avoid
additional lock
> overhead.
> >
> > Signed-off-by: Sumit Saxena
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 57b47fe..c69c1ac 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1065,6 +1065,13 @@ megasas_fire_cmd_fusion(struct
> megasas_instance *instance,
> > u32 req_desc_hi,
> > struct megasas_register_set __iomem *regs)
>
> Hi Sumit,
> the fn params are a bit confusing req_desc_lo is of type dma_addr_t and
> req_desc_hi is u32, is it possible to unite it in the future?
Agree. We should make changes here. We will do it in separate patch.
Originally fire_cmd() was written for MFI controller and carry forward for
all generation.
In MFI it use second argument as 32 bit address and third argument as
frame count, but later in Fusion adapter it started using differently.
>
> > {
> > +#if defined(writeq) && defined(CONFIG_64BIT)
>
> On a similar place mpt2sas(_base_writeq) uses only "#ifndef writeq"
> if it's incorrect fix it there too or remove the CONFIG_64 here
We would like to change at mpt2sas as we have all the code with below
check for writeq()
Original discuss was started when we submitted this change in mpt2sas, but
we have delta from day-1.
LSI/Avago internal source has "#if defined(writeq) &&
defined(CONFIG_64BIT)" check in mpt2sas.
I think now writeq() is implemented in all arch, so we can safely remove
check for #if writeq().
But we can keep this check as it is to continue for older Distribution to
take direct advantage without maintaining any separate patch.
>
> > + u64 req_data = 0;
> > +
> > + req_data = req_desc_hi;
> > + req_data = ((req_data << 32) | (u32)req_desc_lo);
>
> This seems to be critical path (you are removing an spinlock to avoid
> overhead), so why do you have three consecutive assignments to the same
> variable?
> (~(u64 req_data = r_hi << 32 | r_lo))
Agree. We will be doing this change and re-submit the patch to address
this.
>
> Cheers,
> Tomas
>
> > + writeq(le64_to_cpu(req_data), &(regs)-
> >inbound_low_queue_port);
> > +#else
> > unsigned long flags;
> >
> > spin_lock_irqsave(&instance->hba_lock, flags); @@ -1072,6 +1079,7
> @@
> > megasas_fire_cmd_fusion(struct megasas_instance *instance,
> > writel(le32_to_cpu(req_desc_lo), &(regs)-
> >inbound_low_queue_port);
> > writel(le32_to_cpu(req_desc_hi), &(regs)-
> >inbound_high_queue_port);
> > spin_unlock_irqrestore(&instance->hba_lock, flags);
> > +#endif
> > }
> >
> > /**
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature support
On 09/09/2014 06:18 PM, Elliott, Robert (Server Storage) wrote:
>
>> -Original Message-
>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>> ow...@vger.kernel.org] On Behalf Of Tomas Henzl
>> Sent: Tuesday, 09 September, 2014 10:54 AM
>> Subject: Re: [PATCH 04/11] megaraid_sas : Firmware crash dump feature
>> support
>>
>> On 09/06/2014 03:25 PM, sumit.sax...@avagotech.com wrote:
>>> This feature will provide similar interface as kernel crash dump
>> feature.
>>> When megaraid firmware encounter any crash, driver will collect the
>> firmware raw image and
>>> dump it into pre-configured location.
>>>
> ...
>> With several controllers in a system this may take a lot memory,
>> could you also
>> in case when a kdump kernel is running lower it, by not using this
>> feature?
> What is the correct way for a driver to determine that it is
> running in a kdump kernel? The reset_devices global variable?
Yes reset_devices is used for this purpose.
I think, that I've seen some drivers even trying to reduce the normal
mem use, because a top performance is not needed.
>
>
> ---
> Rob ElliottHP Server Storage
>
>
>
> N�r��y���b�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
> ���j:+v���w�j�mzZ+�ݢj"��!tml=
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM
Tom Gundersen schreef op 10-09-14 om 08:46: >Indeed. What I proposed with a multiplier for the timeout for the >different types of built in commands was deemed complex but saw no >alternatives proposed despite my interest to work on one and >clarifications noted that this was a design regression. Not quite sure >what else I could have done here. I'm interested in learning what the >better approach is for the future as if we want to marry init + kernel >we need a smooth way for us to discuss design without getting worked >up about it, or taking it personal. I really want this to work as I >personally like systemd so far. How about this: keep the timeout global, but also introduce a (relatively short, say 10 or 15 seconds) timeout after which a warning is printed. Even if nothing is actually killed, having workers (be it insmod or something else) take longer than a couple of seconds is likely a sign that something is seriously off somewhere. I don't agree with the statement that something is seriously off when it takes more then 10 to 15 seconds. When probing only one hard disk drive, then I do agree that something is seriously off after 10 to 15 seconds. When probing a SAS bus with one hundred hard disk drives in standby mode, then I do expect that to take longer then 10 to 15 seconds. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 16/17] arcmsr: support new adapter ARC12x4 series
On 09/09/2014 06:30 PM, Christoph Hellwig wrote: > Ching, > > do you have a chance to address Thomas second concern below? As > far as I can tell (Thomas, please correct me) that's the last > outstanding concern, and I'd really like to merge the arcmsr updates > for the Linux 3.18 merge window. Correct, still awaiting a response. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
