Re: [PATCH v7] scsi: Add hwmon support for SMART temperature sensors

[Guenter Roeck]

On 11/23/18 12:18 AM, Linus Walleij wrote:

On Thu, Nov 22, 2018 at 9:00 PM Guenter Roeck  wrote:


Can you possibly extract this as pure hwmon driver outside scsi control ?
I'll be happy to accept it as standalone hwmon driver.


That should be last resort, what the SCSI people want is noble,
and they did a tremendous (impressive) work by hiding all the ATA drives
behind SCSI emulation with libata, so they want me to keep up
that tradition by also making the temperature reading behave
"as if it was a SCSI drive" too so I'm on board with trying that out
even if I think the bar is a bit high for causal contributors.



No problem, your call. I would just very much dislike your work
to get lost, that is all.

Thanks,
Guenter

Re: [PATCH v7] scsi: Add hwmon support for SMART temperature sensors

[Guenter Roeck]

[-cc]

Hi Linus,

On 11/22/18 5:49 AM, Linus Walleij wrote:
[ ... ]

(It's
also worth noting that HDD temperature sensors are notoriously
unreliable).


I am sorry if you think that D-Link does bad engineering, what I
am trying to achieve is upstream support for this device, without
any out-of-tree patches. The D-Link DIR-685 uses the harddisk
sensor for this, whether we like it or not.



Following the above argument (not yours, the one about accuracy),
I guess we should drop support for all CPU temperature sensors
from the Linux kernel. After all, they are known to be much more
inaccurate than a HDD temperature sensor could ever be.

Seriously, any argument about (lack of) sensor accuracy should be
silently ignored. I may be overly pessimistic nowadays, but whenever
I see such an argument, I read it as "we'll never accept your patch,
so better stop wasting your time and forget about it". I hope I am
wrong, but it seems to me that this is where things are going.

Can you possibly extract this as pure hwmon driver outside scsi control ?
I'll be happy to accept it as standalone hwmon driver.

Thanks,
Guenter

Re: [PATCH 10/13] timer: Remove expires and data arguments from DEFINE_TIMER

[Guenter Roeck]

On 10/04/2017 04:27 PM, Kees Cook wrote:

Drop the arguments from the macro and adjust all callers with the
following script:

   perl -pi -e 's/DEFINE_TIMER\((.*), 0, 0\);/DEFINE_TIMER($1);/g;' \
 $(git grep DEFINE_TIMER | cut -d: -f1 | sort -u | grep -v timer.h)

Signed-off-by: Kees Cook <keesc...@chromium.org>
Acked-by: Geert Uytterhoeven <ge...@linux-m68k.org> # for m68k parts


For watchdog:

Acked-by: Guenter Roeck <li...@roeck-us.net>


---
  arch/arm/mach-ixp4xx/dsmg600-setup.c  | 2 +-
  arch/arm/mach-ixp4xx/nas100d-setup.c  | 2 +-
  arch/m68k/amiga/amisound.c| 2 +-
  arch/m68k/mac/macboing.c  | 2 +-
  arch/mips/mti-malta/malta-display.c   | 2 +-
  arch/parisc/kernel/pdc_cons.c | 2 +-
  arch/s390/mm/cmm.c| 2 +-
  drivers/atm/idt77105.c| 4 ++--
  drivers/atm/iphase.c  | 2 +-
  drivers/block/ataflop.c   | 8 
  drivers/char/dtlk.c   | 2 +-
  drivers/char/hangcheck-timer.c| 2 +-
  drivers/char/nwbutton.c   | 2 +-
  drivers/char/rtc.c| 2 +-
  drivers/input/touchscreen/s3c2410_ts.c| 2 +-
  drivers/net/cris/eth_v10.c| 6 +++---
  drivers/net/hamradio/yam.c| 2 +-
  drivers/net/wireless/atmel/at76c50x-usb.c | 2 +-
  drivers/staging/speakup/main.c| 2 +-
  drivers/staging/speakup/synth.c   | 2 +-
  drivers/tty/cyclades.c| 2 +-
  drivers/tty/isicom.c  | 2 +-
  drivers/tty/moxa.c| 2 +-
  drivers/tty/rocket.c  | 2 +-
  drivers/tty/vt/keyboard.c | 2 +-
  drivers/tty/vt/vt.c   | 2 +-
  drivers/watchdog/alim7101_wdt.c   | 2 +-
  drivers/watchdog/machzwd.c| 2 +-
  drivers/watchdog/mixcomwd.c   | 2 +-
  drivers/watchdog/sbc60xxwdt.c | 2 +-
  drivers/watchdog/sc520_wdt.c  | 2 +-
  drivers/watchdog/via_wdt.c| 2 +-
  drivers/watchdog/w83877f_wdt.c| 2 +-
  drivers/xen/grant-table.c | 2 +-
  fs/pstore/platform.c  | 2 +-
  include/linux/timer.h | 4 ++--
  kernel/irq/spurious.c | 2 +-
  lib/random32.c| 2 +-
  net/atm/mpc.c | 2 +-
  net/decnet/dn_route.c | 2 +-
  net/ipv6/ip6_flowlabel.c  | 2 +-
  net/netrom/nr_loopback.c  | 2 +-
  security/keys/gc.c| 2 +-
  sound/oss/midibuf.c   | 2 +-
  sound/oss/soundcard.c | 2 +-
  sound/oss/sys_timer.c | 2 +-
  sound/oss/uart6850.c  | 2 +-
  47 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/arch/arm/mach-ixp4xx/dsmg600-setup.c 
b/arch/arm/mach-ixp4xx/dsmg600-setup.c
index b3bd0e137f6d..b3689a141ec6 100644
--- a/arch/arm/mach-ixp4xx/dsmg600-setup.c
+++ b/arch/arm/mach-ixp4xx/dsmg600-setup.c
@@ -174,7 +174,7 @@ static int power_button_countdown;
  #define PBUTTON_HOLDDOWN_COUNT 4 /* 2 secs */
  
  static void dsmg600_power_handler(unsigned long data);

-static DEFINE_TIMER(dsmg600_power_timer, dsmg600_power_handler, 0, 0);
+static DEFINE_TIMER(dsmg600_power_timer, dsmg600_power_handler);
  
  static void dsmg600_power_handler(unsigned long data)

  {
diff --git a/arch/arm/mach-ixp4xx/nas100d-setup.c 
b/arch/arm/mach-ixp4xx/nas100d-setup.c
index 4e0f762bc651..562d05f9888e 100644
--- a/arch/arm/mach-ixp4xx/nas100d-setup.c
+++ b/arch/arm/mach-ixp4xx/nas100d-setup.c
@@ -197,7 +197,7 @@ static int power_button_countdown;
  #define PBUTTON_HOLDDOWN_COUNT 4 /* 2 secs */
  
  static void nas100d_power_handler(unsigned long data);

-static DEFINE_TIMER(nas100d_power_timer, nas100d_power_handler, 0, 0);
+static DEFINE_TIMER(nas100d_power_timer, nas100d_power_handler);
  
  static void nas100d_power_handler(unsigned long data)

  {
diff --git a/arch/m68k/amiga/amisound.c b/arch/m68k/amiga/amisound.c
index 90a60d758f8b..a23f48181fd6 100644
--- a/arch/m68k/amiga/amisound.c
+++ b/arch/m68k/amiga/amisound.c
@@ -66,7 +66,7 @@ void __init amiga_init_sound(void)
  }
  
  static void nosound( unsigned long ignored );

-static DEFINE_TIMER(sound_timer, nosound, 0, 0);
+static DEFINE_TIMER(sound_timer, nosound);
  
  void amiga_mksound( unsigned int hz, unsigned int ticks )

  {
diff --git a/arch/m68k/mac/macboing.c b/arch/m68k/mac/macboing.c
index ffaa1f6439ae..9a52aff183d0 100644
--- a/arch/m68k/mac/macboing.c
+++ b/arch/m68k/mac/macboing.c
@@ -56,7 +56,7 @@ static void ( *mac_special_bell )( unsigned int, unsigned 
int, unsigned int );
  /*
   * our timer to start/continue/stop the bell
   */
-static DEFINE_TIMER(mac_sound_timer, mac_nosound, 0, 0);
+static DEFINE_TIMER(mac_sound_timer, mac_nosound);
  
  /*

   * 

Re: [PATCH 09/13] timer: Remove users of expire and data arguments to DEFINE_TIMER

[Guenter Roeck]

On 10/04/2017 04:27 PM, Kees Cook wrote:

The expire and data arguments of DEFINE_TIMER are only used in two places
and are ignored by the code (malta-display.c only uses mod_timer(),
never add_timer(), so the preset expires value is ignored). Set both
sets of arguments to zero.

Cc: Ralf Baechle <r...@linux-mips.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: Guenter Roeck <li...@roeck-us.net>
Cc: Geert Uytterhoeven <ge...@linux-m68k.org>
Cc: linux-m...@linux-mips.org
Cc: linux-watch...@vger.kernel.org
Signed-off-by: Kees Cook <keesc...@chromium.org>


For watchdog:

Acked-by: Guenter Roeck <li...@roeck-us.net>


---
  arch/mips/mti-malta/malta-display.c | 6 +++---
  drivers/watchdog/alim7101_wdt.c | 4 ++--
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/mips/mti-malta/malta-display.c 
b/arch/mips/mti-malta/malta-display.c
index d4f807191ecd..ac813158b9b8 100644
--- a/arch/mips/mti-malta/malta-display.c
+++ b/arch/mips/mti-malta/malta-display.c
@@ -36,10 +36,10 @@ void mips_display_message(const char *str)
}
  }
  
-static void scroll_display_message(unsigned long data);

-static DEFINE_TIMER(mips_scroll_timer, scroll_display_message, HZ, 0);
+static void scroll_display_message(unsigned long unused);
+static DEFINE_TIMER(mips_scroll_timer, scroll_display_message, 0, 0);
  
-static void scroll_display_message(unsigned long data)

+static void scroll_display_message(unsigned long unused)
  {
mips_display_message(_string[display_count++]);
if (display_count == max_display_count)
diff --git a/drivers/watchdog/alim7101_wdt.c b/drivers/watchdog/alim7101_wdt.c
index 665e0e7dfe1e..3c1f6ac68ea9 100644
--- a/drivers/watchdog/alim7101_wdt.c
+++ b/drivers/watchdog/alim7101_wdt.c
@@ -71,7 +71,7 @@ MODULE_PARM_DESC(use_gpio,
"Use the gpio watchdog (required by old cobalt boards).");
  
  static void wdt_timer_ping(unsigned long);

-static DEFINE_TIMER(timer, wdt_timer_ping, 0, 1);
+static DEFINE_TIMER(timer, wdt_timer_ping, 0, 0);
  static unsigned long next_heartbeat;
  static unsigned long wdt_is_open;
  static char wdt_expect_close;
@@ -87,7 +87,7 @@ MODULE_PARM_DESC(nowayout,
   *Whack the dog
   */
  
-static void wdt_timer_ping(unsigned long data)

+static void wdt_timer_ping(unsigned long unused)
  {
/* If we got a heartbeat pulse within the WDT_US_INTERVAL
 * we agree to ping the WDT

Re: [PATCH 15/22] hwmon: applesmc: fix format string overflow

[Guenter Roeck]

On 07/14/2017 05:07 AM, Arnd Bergmann wrote:

gcc-7 warns that the key might exceed five bytes for lage index
values:

drivers/hwmon/applesmc.c: In function 'applesmc_show_fan_position':
drivers/hwmon/applesmc.c:906:18: error: '%d' directive writing between 1 and 5 
bytes into a region of size 4 [-Werror=format-overflow=]
sprintf(newkey, FAN_ID_FMT, to_index(attr));
 ^~~
drivers/hwmon/applesmc.c:906:18: note: directive argument in the range [0, 
65535]
drivers/hwmon/applesmc.c:906:2: note: 'sprintf' output between 5 and 9 bytes 
into a destination of size 5

As the key is required to be four characters plus trailing zero,
we know that the index has to be small here. I'm using snprintf()
to avoid the warning. This would truncate the string instead of
overflowing.

Signed-off-by: Arnd Bergmann 


I submitted a more comprehensive patch a couple of days ago. There are other 
similar
sprintf() calls in the driver which gcc doesn't report.

Guenter


---
  drivers/hwmon/applesmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 0af7fd311979..515163b9a89f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -903,7 +903,7 @@ static ssize_t applesmc_show_fan_position(struct device 
*dev,
char newkey[5];
u8 buffer[17];
  
-	sprintf(newkey, FAN_ID_FMT, to_index(attr));

+   snprintf(newkey, sizeof(newkey), FAN_ID_FMT, to_index(attr));
  
  	ret = applesmc_read_key(newkey, buffer, 16);

buffer[16] = 0;

[PATCH] cxlflash: Select IRQ_POLL

[Guenter Roeck]

The driver now uses IRQ_POLL and needs to select it to avoid the following
build error.

ERROR: ".irq_poll_complete" [drivers/scsi/cxlflash/cxlflash.ko] undefined!
ERROR: ".irq_poll_sched" [drivers/scsi/cxlflash/cxlflash.ko] undefined!
ERROR: ".irq_poll_disable" [drivers/scsi/cxlflash/cxlflash.ko] undefined!
ERROR: ".irq_poll_init" [drivers/scsi/cxlflash/cxlflash.ko] undefined!

Fixes: cba06e6de403 ("scsi: cxlflash: Implement IRQ polling for RRQ processing")
Signed-off-by: Guenter Roeck <li...@roeck-us.net>
---
 drivers/scsi/cxlflash/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/cxlflash/Kconfig b/drivers/scsi/cxlflash/Kconfig
index c052104e523e..a011c5dbf214 100644
--- a/drivers/scsi/cxlflash/Kconfig
+++ b/drivers/scsi/cxlflash/Kconfig
@@ -5,6 +5,7 @@
 config CXLFLASH
tristate "Support for IBM CAPI Flash"
depends on PCI && SCSI && CXL && EEH
+   select IRQ_POLL
default m
help
  Allows CAPI Accelerated IO to Flash
-- 
2.7.4

[PATCH -next] advansys: Add missing include file

[Guenter Roeck]

Fix:

drivers/scsi/advansys.c: In function 'adv_isr_callback':
drivers/scsi/advansys.c:6089:3: error:
implicit declaration of function 'dma_pool_free'
drivers/scsi/advansys.c: In function 'adv_get_sglist':
drivers/scsi/advansys.c:7654:3: error:
implicit declaration of function 'dma_pool_alloc'
drivers/scsi/advansys.c: In function 'advansys_wide_init_chip':
drivers/scsi/advansys.c:10866:2: error:
implicit declaration of function 'dma_pool_create'

Fixes: 0ce538226b7a (advansys: Use dma_pool for sg elements)
Cc: Hannes Reinecke h...@suse.de
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
 drivers/scsi/advansys.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 14d3aa54daa5..af2cb5e9e3f7 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -36,6 +36,7 @@
 #include linux/pci.h
 #include linux/spinlock.h
 #include linux/dma-mapping.h
+#include linux/dmapool.h
 #include linux/firmware.h
 
 #include asm/io.h
-- 
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: BUG in scsi_lib.c due to a bad commit

[Guenter Roeck]

On 11/11/2014 04:17 PM, Bjorn Helgaas wrote:

[+cc Guenter, linux-scsi]

On Tue, Nov 11, 2014 at 4:33 PM, Barto mister.free...@laposte.net wrote:

Hello everyone,

I notice a bug since kernel 3.17 ( and also with 3.18 branch ), a random
hang at boot on some PC configurations, I did a git bisect and I found
that the culprit is  :

[045065d8a300a37218c548e9aa7becd581c6a0e8] [SCSI] fix qemu boot hang problem

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=045065d8a300a37218c548e9aa7becd581c6a0e8

the author of this commit has choosen to inverse the logic of the if
statement in the file  drivers/scsi/scsi_lib.c in order to solve an
issue with qemu :

--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1774,7 +1774,7 @@ static void scsi_request_fn(struct request_queue *q)
blk_requeue_request(q, req);
atomic_dec(sdev-device_busy);
out_delay:
- if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
+ if (!atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);
}



The above commit was a follow-up to commit 71e75c97f97a, which did the 
following:

-   if (sdev-device_busy == 0  !scsi_device_blocked(sdev))
+   if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))

meaning it reversed the polarity of the check in the original code.
Since that commit created problems as observed in qemu, I thought it
would be obvious that restoring the original polarity would make sense.
This is explained in my patch, so I am somewhat at loss why ...


this change triggers a bug on my PC ( I don't have SCSI devices, but
only 3 SATA harddisks and 2 IDE harddisks, SATA disks are on an ICH7
sata controler on the motherboard gigabyte GA-P31-DS3L, and IDE disk on
a JMicron JMB363/368 Sata/IDE PCIe card ), every 5~10 boots the boot
stops suddenly because of this commit,

If I revert this commit then the bug is gone, more details can be found
here, where I created a patch who reverts commit 045065d8 :

https://bugzilla.kernel.org/show_bug.cgi?id=87581

my question: why Guenter Roeck ( the author of the bad commit ) has
choosen to inverse the logic in the if statement ?


this question is asked.

Having said that, I don't really care one way or another. I am fine
with reverting my commit; if that is done, I'll simply remove the
related scsi tests from my qemu test runs.

Guenter


before his commit the if statement was like this :

if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);

if his decision to inverse the logic of
atomic_read(sdev-device_busy) is acceptable then the real bug is
probably located elsewhere in the scsi source code, and we must solve
this mistery because there is obviously a bug regression in SCSI code
because with older kernels ( 3.16.7 and lower ) I don't have the random
hang boot bug with my configuration,

another user in archlinux forums has also this bug and he has a more
modern PC ( intel i7 core cpu, SSD device ), my fear is when linux
distros will move to kernel 3.17 then more users will have this weird
random bug who can occur only on boot and only with a specific PC
configuration, if the boot step is passed despite the random bug then
the bug will not occur, it occurs only during the boot process, which
probably means that the faulty source code is only called during the
boot process,

thanks for anyone who wants to dig this problem with me
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/




--
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] scsi: Fix qemu boot hang problem

[Guenter Roeck]

ping ... the problem fixed by this patch still affects the upstream kernel
(v3.16-11383-gc9d2642) as well as -next (20140815).

Guenter

On 08/10/2014 05:54 AM, Guenter Roeck wrote:

The latest kernel fails to boot qemu arm images when using scsi
for disk access. Boot gets stuck after the following messages.

brd: module loaded
sym53c8xx :00:0c.0: enabling device (0100 - 0103)
sym0: 895a rev 0x0 at pci :00:0c.0 irq 93
sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
sym0: SCSI BUS has been reset.
scsi host0: sym-2.2.3

Bisect points to commit 71e75c97f97a (scsi: convert device_busy to
atomic_t). Code inspection shows the following suspicious change
in scsi_request_fn.

out_delay:
-   if (sdev-device_busy == 0  !scsi_device_blocked(sdev))
+   if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);
}

'sdev-device_busy == 0' was replaced with 'atomic_read(sdev-device_busy)',
meaning the logic was reversed. Changing this expression to
'!atomic_read(sdev-device_busy)' fixes the problem.

Cc: Christoph Hellwig h...@lst.de
Cc: Hannes Reinecke h...@suse.de
Cc: Webb Scales web...@hp.com
Cc: Jens Axboe ax...@kernel.dk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
  drivers/scsi/scsi_lib.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9c44392..ce62e87 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1774,7 +1774,7 @@ static void scsi_request_fn(struct request_queue *q)
blk_requeue_request(q, req);
atomic_dec(sdev-device_busy);
  out_delay:
-   if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
+   if (!atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);
  }




--
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] scsi: Fix qemu boot hang problem

[Guenter Roeck]

On 08/15/2014 12:22 PM, Christoph Hellwig wrote:

On Fri, Aug 15, 2014 at 12:34:22PM -0600, Jens Axboe wrote:

On 2014-08-15 12:22, Guenter Roeck wrote:

ping ... the problem fixed by this patch still affects the upstream kernel
(v3.16-11383-gc9d2642) as well as -next (20140815).


James, could you upstream this one in time for -rc1?


It's in the core-for-3.17 updates I sent to James yesterday, just needs to
be forwarded..



Great, thanks a lot!

Guenter

--
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 v2] uas: replace WARN_ON_ONCE() with lockdep_assert_held()

[Guenter Roeck]

On Tue, Aug 12, 2014 at 02:01:53PM +0800, Greg KH wrote:
 On Tue, Aug 12, 2014 at 11:38:37AM +0530, Sanjeev Sharma wrote:
  spin_is_locked() always return false in uniprocessor configuration and 
  therefore it
  would be advise to replace with lockdep_assert_held().
 
 Add on some architectures in here somewhere, as it's not broken on the
 large majority of UP cpus :)
 
FWIW, it is confirmed broken on mips (32 and 64 bit), ppc, and sparc64.
I have not tested on x86. Might be worth trying. arm64 seems to be ok,
unless I did something wrong in my test, as well as at least some of
the architectures which don't support smp to start with (such as sparc32
or microblaze).

Guenter
--
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] scsi: Fix qemu boot hang problem

[Guenter Roeck]

On 08/11/2014 08:33 AM, Venkatesh Srinivas wrote:

Should we wrap the sdev-device_busy read and test in a new
scsi_device_busy(), so as to preserve the old polarity?


I don't see a difference in polarity, or at least replacing '== 0' with '!'
is not a reverse in polarity for me. Anyway, I don't really care if
or how this is wrapped, as long as it is getting fixed.

Thanks,
Guenter

--
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] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Guenter Roeck]

On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
 spin_is_locked() always return false in uniprocessor configuration and 
 therefore it
 would be advise to repalce with assert_spin_locked().
 
 Signed-off-by: Sanjeev Sharma sanjeev_sha...@mentor.com
 ---
  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..8e5877d 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));
 + assert_spin_locked(devinfo-lock);

Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
creates a traceback, and only once) with assert_spin_locked (which
crashes the kernel) is a bit drastic.

Guenter
--
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] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Guenter Roeck]

On Mon, Aug 11, 2014 at 08:55:07PM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/11/2014 08:19 PM, Guenter Roeck wrote:
  On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
  spin_is_locked() always return false in uniprocessor configuration and 
  therefore it
  would be advise to repalce with assert_spin_locked().
 
  Signed-off-by: Sanjeev Sharma sanjeev_sha...@mentor.com
  ---
   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..8e5877d 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));
  +  assert_spin_locked(devinfo-lock);
  
  Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
  creates a traceback, and only once) with assert_spin_locked (which
  crashes the kernel) is a bit drastic.
 
 I can see your point, but so far these paranoia checks have never triggered,
 and having them trigger _always_ one some uni-processor (which is the reason
 for this patch) to me seems the worse problem of the 2.
 
If those are just paranoia checks, it might make sense to use
lockdep_assert_held() to reduce runtime overhead if lockdep
debugging is disabled.

 Ideally we would have a warn_spin_not_locked or such ...
 
There is WARN_ON_SMP, which might have been a better choice if the _ONCE isn't
that important (which one should think if it is ok to crash the kernel if the
problem is seen).

Guenter
--
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] scsi: Fix qemu boot hang problem

[Guenter Roeck]

The latest kernel fails to boot qemu arm images when using scsi
for disk access. Boot gets stuck after the following messages.

brd: module loaded
sym53c8xx :00:0c.0: enabling device (0100 - 0103)
sym0: 895a rev 0x0 at pci :00:0c.0 irq 93
sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
sym0: SCSI BUS has been reset.
scsi host0: sym-2.2.3

Bisect points to commit 71e75c97f97a (scsi: convert device_busy to
atomic_t). Code inspection shows the following suspicious change
in scsi_request_fn.

out_delay:
-   if (sdev-device_busy == 0  !scsi_device_blocked(sdev))
+   if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);
}

'sdev-device_busy == 0' was replaced with 'atomic_read(sdev-device_busy)',
meaning the logic was reversed. Changing this expression to
'!atomic_read(sdev-device_busy)' fixes the problem.

Cc: Christoph Hellwig h...@lst.de
Cc: Hannes Reinecke h...@suse.de
Cc: Webb Scales web...@hp.com
Cc: Jens Axboe ax...@kernel.dk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9c44392..ce62e87 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1774,7 +1774,7 @@ static void scsi_request_fn(struct request_queue *q)
blk_requeue_request(q, req);
atomic_dec(sdev-device_busy);
 out_delay:
-   if (atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
+   if (!atomic_read(sdev-device_busy)  !scsi_device_blocked(sdev))
blk_delay_queue(q, SCSI_QUEUE_DELAY);
 }
 
-- 
1.9.1

--
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