[PATCH, resend] sd: fix infinite kernel/udev loop on non-removable Medium Not Present

[Steve Magnani]

Commit eface65c336eff420d70beb0fb6787a732e05ffb (2.6.38) altered
set_media_not_present() in a way that prevents the sd driver from
remembering that a non-removable device has reported Medium Not Present.
This condition can occur on hotplug of a (i.e.) USB Mass Storage device
whose medium is offline due to an unrecoverable controller error,
but which is otherwise capable of SCSI communication (to download new 
microcode, etc.).

Under these conditions, the changed code results in an infinite loop
between the kernel and udevd. When udevd attempts to open the device
in response to a change notification, a SCSI Medium Not Present error
occurs which causes the kernel to signal another change. The cycle
repeats until the device is unplugged, resulting in udevd consuming ever-
increasing amounts of CPU and virtual memory.

Resolve this by remembering media not present whether the device has
declared itself removable or not.

Signed-off-by: Steven J. Magnani st...@digidescorp.com
---
--- a/drivers/scsi/sd.c 2013-04-12 14:16:12.252531097 -0500
+++ b/drivers/scsi/sd.c 2013-04-12 14:21:55.197216521 -0500
@@ -1298,10 +1298,8 @@ out:
 
 static void set_media_not_present(struct scsi_disk *sdkp)
 {
-   if (sdkp-media_present)
+   if (sdkp-media_present) {
sdkp-device-changed = 1;
-
-   if (sdkp-device-removable) {
sdkp-media_present = 0;
sdkp-capacity = 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: READ_CAPACITY_16 vs. READ_CAPACITY_10

[Steve Magnani]

On Wed, 2013-09-11 at 11:42 -0400, Alan Stern wrote: 
 On Wed, 11 Sep 2013, Oliver Neukum wrote:
  
  I'll try to get a Windows machine for a trace.
  Can you suggest a tracer for Win7?
 
 I don't know of any, offhand.  Maybe Google can help.
 
 Alternatively, you could install Windows 7 in a virtual machine under 
 Linux and use usbmon.

USBPcap can capture Windows USB transactions which Wireshark 1.10 or
later can interpret:
http://desowin.org/usbpcap/ 
Note, if you're used to WinPCap or usbmon capture integration with
Wireshark this will be an adjustment. You have to run USBPcap from the
command line to do the capture to a file, quit the capture, then open
the file in Wireshark.

I think depending on the options you pass to USBPcap you could end up
with .pcap frames that are larger than Wireshark can handle (i.e., for a
READ(x) transfer of 128 blocks).

Regards,

Steven J. Magnani   I claim this network for MARS!
www.digidescorp.com  Earthling, return my space modulator!

#include standard.disclaimer


--
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: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled

[Steve Magnani]

Nicholas,

On Sat, 2013-12-14 at 10:51 -0600, Dr. Greg Wettstein wrote:
 On Dec 12, 11:45am, Nicholas A. Bellinger wrote:
   What I would prefer myself is to have a single set of target drivers
   that works for both LIO and SCST. That would not only make both projects
   easier to maintain but also would expose all target drivers to wider
   testing. However, since the interface between target core and target
   drivers is very different for LIO and for SCST reaching this goal would
   be a challenging project by itself.
 
  After spending 18+ months on qla2xxx target code, and getting it
  into a state that it could withstand public scrutiny for mainline
  acceptance, I have no interest in making changes to mainline driver
  code for supporting someone else's out-of-tree stuff.
 
 I believe, to be fair, that if it took 18 months to get the qla2x00t
 code ported into the kernel that it would have taken even longer to
 get fibre-channel support for the in-kernel target stack if the
 process would have started from scratch.  The kernel implementation
 stands very heavily on engineering work which Vlad, ID7 and others put
 into the target code.
 
 Secondary to implementing the sqatgt driver I have been through every
 line of both Vlad's code and your derivative work.  It is a straight
 forward exercise to follow both pieces of work side by side.
 
 So despite all the hostility and rancor which surrounds the two major
 Linux SCSI target implementations, lets concede that the in-kernel
 fibre-channel driver benefited a great deal from the efforts of the
 SCST community.  This coming from someone who arranged for a small
 amount of financial support for some of the code which is currently
 residing in the in-kernel Qlogic target driver.
 
  We don't add interfaces into mainline drive code to support
  out-of-tree projects, because quite frankly, it gives less incentive
  for the out-of-tree holdouts to use, improve and contribute to
  mainline target code.
 
 But the kernel does provide services to external users through a
 binary API supplied by exported symbols.  Your work on the Qlogic
 fibre-channel target driver includes all but one small function needed
 to allow SCST to use the in kernel driver.
 
 To Nicholas' credit, the engineering work done on the qla2x00t driver,
 as a component of porting it into the kernel, provides almost all of
 the resources needed for a straight forward integration of SCST.
 Having anticipated this debate we focused on designing the sqatgt
 interface driver so it would integrate cleanly with the architectural
 model selected for the kernel.
 
 Bart, if you could review the in-kernel Qlogic driver you will find
 the the tcm_qla2xxx.c module interfaces the Qlogic driver with the
 in-kernel SCSI target engine.  Similar functionality is provided by
 the scst_qla2xxx.c in the sqatgt driver for SCST.  So there is a
 reasonably straight forward and standardized approach for getting both
 projects on a common driver infrastructure, which as you note benefits
 everyone.
 
 The only missing piece of the puzzle is an exported function to
 enumerate the target interfaces which are available.  Since there are
 exported functions to enable/disable interfaces, handle session
 management and a variety of other functions it doesn't seem
 unreasonable to provide a method of enumerating which interfaces are
 available for target mode.
 
 So Nicholas in the spirit of everyone working for the common good of
 the storage community I'm hoping you will support our proposal of the
 following patch to Andrew Vasquez and the Qlogic maintainers:
 
 ---
 +void
 +qlt_get_target_list(void (*callback)(struct scsi_qla_host *))
 +{
 +   struct qla_tgt *tgt;
 +
 +mutex_lock(qla_tgt_mutex);
 +list_for_each_entry(tgt, qla_tgt_glist, tgt_list_entry) {
 +   (*callback)(tgt-vha);
 +   }
 +mutex_unlock(qla_tgt_mutex);
 +
 +   return;
 +}
 +EXPORT_SYMBOL(qlt_get_target_list);
 ---
 
 Which provides the only missing piece needed for SCST to live happily
 on as a modular extension to the kernel.  With this in place we can
 all focus on more pressing issues, such as why Steve Magnani can't get
 two ports on the same ISP chip to run in different modes.

I have code from QLogic that appears to resolve the issue with which I
started this thread, namely crummy initiator performance when the same
physical port is configured for dual initiator and target mode. QLogic's
code appears to be a fusion of the mainline qla2xxx driver and SCST,
which makes it taxonomically close to Greg's sqatgt proposal.

Once I figure out which portions of QLogic's code resolve the issue I
would be happy to post them as patches to mainline. But I can't justify
the effort to do that if Greg's proposal is DOA, since I wouldn't be
able to make use

Re: Why is (2 2) true? Is it a gcc bug?

[Steve Magnani]

On 01/17/2014 08:55 AM, Dorau, Lukasz wrote:


On Friday, January 17, 2014 2:58 PM Richard Weinberger 
richard.weinber...@gmail.com wrote:

Can you reproduce this using a standalone test?
I.e:
#include assert.h

int main()
{
 assert(2  2 != 1);

 return 0;
}


No, I can't of course.


The test might need to be more complex in order to prevent the compiler from 
optimizing away the issue. Or, you could try -O0.



 Steven J. Magnani   I claim this network for MARS!

 www.digidescorp.com  Earthling, return my space modulator!

 #include standard.disclaimer

--
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] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Steve Magnani]

On 02/27/2017 12:57 PM, Bart Van Assche wrote:

...
How about the (untested) patch below? The approach below avoids that the check 
is
duplicated and - at least in my opinion - results in code that is easier to 
read.
I find lba_too_large() a little dense, but functionally OK. The "shift 
>= 0" clause could be dropped.

I tested this on my "problem" system (READ CAPACITY(10)) without incident.


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..3533d1e46bde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2082,6 +2082,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
sdkp->capacity = 0; /* unknown mapped to zero - as usual */
  }
  
+/*

+ * Check whether or not logical_to_sectors(sdp, lba) will overflow.
+ */
+static bool lba_too_large(u64 lba, u32 logical_block_size)
+{
+   int shift = sizeof(sector_t) * 8 + 9 - ilog2(logical_block_size);
+
+   return shift >= 0 && shift < 64 && lba >= (1ULL << shift);
+}
+
  #define RC16_LEN 32
  #if RC16_LEN > SD_BUF_SIZE
  #error RC16_LEN must not be more than SD_BUF_SIZE
@@ -2154,7 +2164,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   if (lba_too_large(lba + 1, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2243,7 +2253,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   if (lba_too_large(lba + 1ULL, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Steve Magnani]

Hi Bart -

Thanks for taking the time to look this over.

On 02/27/2017 10:13 AM, Bart Van Assche wrote:

On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:

@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = lba << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   ((lba >= 0xULL) || (lba_in_sectors >= 0xULL))) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
int the_result;
int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
sector_t lba;
+   unsigned long long lba_in_sectors;
unsigned sector_size;
  
  	do {

@@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   (lba_in_sectors >= 0xULL)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Why are the two checks slightly different? Could the same code be used for
both checks?
The checks are different because with READ CAPACITY(16) a _really_ huge 
device could report a max LBA so large that left-shifting it causes bits 
to drop off the end. That's not an issue with READ CAPACITY(10) because 
at most the 32-bit LBA reported by the device will become a 35-bit value 
(since the max supported block size is 4096 == (512 << 3)).



BTW, using the macro below would make the above checks less
verbose and easier to read:

/*
  * Test whether the result of a shift-left operation would be larger than
  * what fits in a variable with the type of @a.
  */
#define shift_left_overflows(a, b)  \
({  \
typeof(a) _minus_one = -1LL;\
typeof(a) _plus_one = 1;\
bool _a_is_signed = _minus_one < 0;  \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed);  \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})

Bart.
Perhaps but I am not a fan of putting braces in non-obvious places such 
as within array subscripts (which I've encountered recently) or 
conditional expressions, which is what this amounts to.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include 

RE: [SCSI] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

[Steve Magnani]

Bart -

I've been porting patches from mainline into a copy of QLogic's target driver 
tree
and ran across an anomaly in this changeset of yours from 2013
(8c0eb596baa51f2b43949c698c644727ef17805c).


The commit log says:

... Make it easy for Coverity (and for humans) to recognize that there is no
fcport leak in the error path by changing the

  bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN

test into

  bsg_job->request->msgcode != FC_BSG_RPT_ELS.


But the change actually made was this:

@@ -399,7 +399,7 @@ done_unmap_sg:
goto done_free_fcport;
 
 done_free_fcport:

-   if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
+   if (bsg_job->request->msgcode == FC_BSG_RPT_ELS)
kfree(fcport);
 done:
return rval;


Shouldn't the "== FC_BSG_RPT_ELS" be "!= FC_BSG_RPT_ELS"?

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include 

Re: Sniffing FC traffic

[Steve Magnani]

On 08/18/2017 08:31 AM, Laurence Oberman wrote:

On Fri, Aug 18, 2017 at 8:37 AM, Thomas Glanzmann  wrote:

I would like to create a setup that allows me to sniff FC traffic. Is it
possible with Linux or can someone recommend a setup that works. I want
to avoid buying a 120kUSD fabric analyzer.


There is no way to do this using adapters and generic F/C with Linux as the O/S.
The ability to enable debugging in the F/C drivers will expose some of
the internals but there is no way to sniff directly as far as I am
aware.

Many switches allow port level tracing facilities but inline sniffing
using Linux and generic hosts is not possible.

We have Finisars for inline tracing when we have to debug host and
fabric issues.

Software based FCOE using libfc and the Intel cards for example will
allow Wireshark tracing but that is encapsulated F/C in Ethernet
packets hence the Wireshark ability.


We have had some success using a (Teledyne) LeCroy analyzer. Its GUI 
(SierraNet) runs only on a Windows host, but it is capable of exporting 
a capture to Wireshark. This mostly shows just the ELS and FCP 
transactions; to debug low-level link issues you'd have to work with the 
original capture in SierraNet.


Like any other kind of analyzer the cost will be proportional to the 
speed of the link you're trying to sniff and the amount of capture depth 
you need. You may be able to save some money by getting secondhand 
equipment and/or running the link at lower speed when you need to debug 
something.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
www.digidescorp.com Earthling, return my space modulator!"

 #include