-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Dan Williams]

The commit below causes the libnvdimm sub-system to stop loading.
This is due to the fact that nvdimm_bus_match() returns the result of
test_bit() which may be negative.  If there are any other bus match
functions using test_bit they may be similarly impacted.

Can we queue a fixup like the following to libnvdimm, and maybe
others, ahead of this driver core change?

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f701bc..2b2181cdeb63 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev,
struct device_driver *drv)
{
       struct nd_device_driver *nd_drv = to_nd_device_driver(drv);

-       return test_bit(to_nd_device_type(dev), &nd_drv->type);
+       return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
}

static struct module *to_bus_provider(struct device *dev)




Other ideas?

commit 09a14906a26e454cad7ff0ad96af40fc4cd90eb0
Author: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Date:   Tue Dec 8 10:00:45 2015 +0100

   ARM: 8472/1: driver cohandle -EPROBE_DEFER from bus_type.match()

   Allow implementations of the match() callback in struct bus_type to
   return errors and if it's -EPROBE_DEFER then queue the device for
   deferred probing.

   This is useful to buses such as AMBA in which devices are registered
   before their matching information can be retrieved from the HW
   (typically because a clock driver hasn't probed yet).

   [changed if-else code structure, adjusted documentation to match the code,
   extended comments]

   Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
   Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
   Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
   Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Dan Williams]

On Thu, Dec 17, 2015 at 7:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> The commit below causes the libnvdimm sub-system to stop loading.
> This is due to the fact that nvdimm_bus_match() returns the result of
> test_bit() which may be negative.  If there are any other bus match
> functions using test_bit they may be similarly impacted.
>
> Can we queue a fixup like the following to libnvdimm, and maybe
> others, ahead of this driver core change?
>
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 7e2c43f701bc..2b2181cdeb63 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev,
> struct device_driver *drv)
> {
>        struct nd_device_driver *nd_drv = to_nd_device_driver(drv);
>
> -       return test_bit(to_nd_device_type(dev), &nd_drv->type);
> +       return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
> }
>
> static struct module *to_bus_provider(struct device *dev)
>
>
>
>
> Other ideas?

How about just checking for EPROBE_DEFER, every other non-zero value is success.

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Matthew Wilcox]

On Thu, Dec 17, 2015 at 08:48:09AM -0800, Dan Williams wrote:
> On Thu, Dec 17, 2015 at 7:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> How about just checking for EPROBE_DEFER, every other non-zero value is success.

How about using IS_ERR_VALUE()?  If it's good enough for IS_ERR(),
it's good enough for this case.

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Ross Zwisler]

On Thu, Dec 17, 2015 at 8:51 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> The commit below causes the libnvdimm sub-system to stop loading.
> This is due to the fact that nvdimm_bus_match() returns the result of
> test_bit() which may be negative.  If there are any other bus match
> functions using test_bit they may be similarly impacted.
>
> Can we queue a fixup like the following to libnvdimm, and maybe
> others, ahead of this driver core change?
>
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 7e2c43f701bc..2b2181cdeb63 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev,
> struct device_driver *drv)
> {
>        struct nd_device_driver *nd_drv = to_nd_device_driver(drv);
>
> -       return test_bit(to_nd_device_type(dev), &nd_drv->type);
> +       return !!test_bit(to_nd_device_type(dev), &nd_drv->type);

How is this call to test_bit() returning a negative value?  That can
only happen if the bit number we supply is 63, correct?  I only see
to_nd_device_type() returning 1-6 for the defines ND_DEVICE_DIMM thru
ND_DEVICE_NAMESPACE_BLK?

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Russell King - ARM Linux]

On Thu, Dec 17, 2015 at 07:51:14AM -0800, Dan Williams wrote:
> The commit below causes the libnvdimm sub-system to stop loading.
> This is due to the fact that nvdimm_bus_match() returns the result of
> test_bit() which may be negative.  If there are any other bus match
> functions using test_bit they may be similarly impacted.
> 
> Can we queue a fixup like the following to libnvdimm, and maybe
> others, ahead of this driver core change?

This is rather annoying.  Have we uncovered a latent bug in other
architectures?  Well, looking through the test_bit() implementations,
it looks like it.

I'll drop the patch set for the time being, we can't go around breaking
stuff like this.  However, I think the test_bit() result should be
regularised across different architectures - it _looks_ to me like most
implementations return 0/1 values, but there may be some that don't
(maybe the assembly versions?)

Here's the list I've pulled out so far from the "easy" cases, which all
look like they're returning 0/1 values.

asm-generic: 0/1

/**
 * test_bit - Determine whether a bit is set
 * @nr: bit number to test
 * @addr: Address to start counting from
 */
static inline int test_bit(int nr, const volatile unsigned long *addr)
{
        return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
}

alpha: 0/1

static inline int
test_bit(int nr, const volatile void * addr)
{
        return (1UL & (((const int *) addr)[nr >> 5] >> (nr & 31))) != 0UL;
}

arm: 0/1

test_bit(unsigned int nr, const volatile unsigned long *addr)
{
        unsigned long mask;

        addr += nr >> 5;

        mask = 1UL << (nr & 0x1f);

        return ((mask & *addr) != 0);
}

blackfin: 0/1

static inline int test_bit(int nr, const volatile unsigned long *addr)
{
        volatile const unsigned long *a = addr + (nr >> 5);
        return __raw_bit_test_asm(a, nr & 0x1f) != 0;
}

frv: 0/1

static inline int
__constant_test_bit(unsigned long nr, const volatile void *addr)
{
        return ((1UL << (nr & 31)) & (((const volatile unsigned int *) addr)[nr >> 5])) != 0;
}
(and similar for __test_bit)

h8300 uses assembly... no idea

hexagon uses assembly as well... no idea

ia64: 0/1

static __inline__ int
test_bit (int nr, const volatile void *addr)
{
        return 1 & (((const volatile __u32 *) addr)[nr >> 5] >> (nr & 31));
}

m68k: 0/1

static inline int test_bit(int nr, const unsigned long *vaddr)
{
        return (vaddr[nr >> 5] & (1UL << (nr & 31))) != 0;
}

mn10300: 0/1

static inline int test_bit(unsigned long nr, const volatile void *addr)
{
        return 1UL & (((const volatile unsigned int *) addr)[nr >> 5] >> (nr & 31));
}

s390: 0/1

static inline int test_bit(unsigned long nr, const volatile unsigned long *ptr)
{
        const volatile unsigned char *addr;

        addr = ((const volatile unsigned char *)ptr);
        addr += (nr ^ (BITS_PER_LONG - 8)) >> 3;
        return (*addr >> (nr & 7)) & 1;
}

x86: 0/1 for constant, ? for variable

static __always_inline int constant_test_bit(long nr, const volatile unsigned long *addr)
{
        return ((1UL << (nr & (BITS_PER_LONG-1))) &
                (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
}
(presumably variable_test_bit is the same, but I don't know)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Dan Williams]

[Adding Dave Howells who tried to correct this situation earlier this year]

On Thu, Dec 17, 2015 at 10:46 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Dec 17, 2015 at 07:51:14AM -0800, Dan Williams wrote:
>> The commit below causes the libnvdimm sub-system to stop loading.
>> This is due to the fact that nvdimm_bus_match() returns the result of
>> test_bit() which may be negative.  If there are any other bus match
>> functions using test_bit they may be similarly impacted.
>>
>> Can we queue a fixup like the following to libnvdimm, and maybe
>> others, ahead of this driver core change?
>
> This is rather annoying.  Have we uncovered a latent bug in other
> architectures?  Well, looking through the test_bit() implementations,
> it looks like it.
>
> I'll drop the patch set for the time being, we can't go around breaking
> stuff like this.

...or make the interpretation from the return value of ->match() be 0,
-EPROBE_DEFER, or other non-zero value for success?  Although that's
fairly subtle.

> However, I think the test_bit() result should be
> regularised across different architectures - it _looks_ to me like most
> implementations return 0/1 values, but there may be some that don't
> (maybe the assembly versions?)

Correct.  Al the constant versions return 0 or 1, but the assembly
return 0 or non-zero.

Here's a link to Dave's rework.

https://lwn.net/Articles/642437/

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Yoshinori Sato]

On Fri, 18 Dec 2015 03:46:41 +0900,
Russell King - ARM Linux wrote:
> 
> On Thu, Dec 17, 2015 at 07:51:14AM -0800, Dan Williams wrote:
> > The commit below causes the libnvdimm sub-system to stop loading.
> > This is due to the fact that nvdimm_bus_match() returns the result of
> > test_bit() which may be negative.  If there are any other bus match
> > functions using test_bit they may be similarly impacted.
> > 
> > Can we queue a fixup like the following to libnvdimm, and maybe
> > others, ahead of this driver core change?
> 
> This is rather annoying.  Have we uncovered a latent bug in other
> architectures?  Well, looking through the test_bit() implementations,
> it looks like it.
> 
> I'll drop the patch set for the time being, we can't go around breaking
> stuff like this.  However, I think the test_bit() result should be
> regularised across different architectures - it _looks_ to me like most
> implementations return 0/1 values, but there may be some that don't
> (maybe the assembly versions?)
> 
> Here's the list I've pulled out so far from the "easy" cases, which all
> look like they're returning 0/1 values.
> 
> asm-generic: 0/1
> 
> /**
>  * test_bit - Determine whether a bit is set
>  * @nr: bit number to test
>  * @addr: Address to start counting from
>  */
> static inline int test_bit(int nr, const volatile unsigned long *addr)
> {
>         return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
> }
> 
> alpha: 0/1
> 
> static inline int
> test_bit(int nr, const volatile void * addr)
> {
>         return (1UL & (((const int *) addr)[nr >> 5] >> (nr & 31))) != 0UL;
> }
> 
> arm: 0/1
> 
> test_bit(unsigned int nr, const volatile unsigned long *addr)
> {
>         unsigned long mask;
> 
>         addr += nr >> 5;
> 
>         mask = 1UL << (nr & 0x1f);
> 
>         return ((mask & *addr) != 0);
> }
> 
> blackfin: 0/1
> 
> static inline int test_bit(int nr, const volatile unsigned long *addr)
> {
>         volatile const unsigned long *a = addr + (nr >> 5);
>         return __raw_bit_test_asm(a, nr & 0x1f) != 0;
> }
> 
> frv: 0/1
> 
> static inline int
> __constant_test_bit(unsigned long nr, const volatile void *addr)
> {
>         return ((1UL << (nr & 31)) & (((const volatile unsigned int *) addr)[nr >> 5])) != 0;
> }
> (and similar for __test_bit)
> 
> h8300 uses assembly... no idea
0/1
I think same return of other architecture.

> hexagon uses assembly as well... no idea
> 
> ia64: 0/1
> 
> static __inline__ int
> test_bit (int nr, const volatile void *addr)
> {
>         return 1 & (((const volatile __u32 *) addr)[nr >> 5] >> (nr & 31));
> }
> 
> m68k: 0/1
> 
> static inline int test_bit(int nr, const unsigned long *vaddr)
> {
>         return (vaddr[nr >> 5] & (1UL << (nr & 31))) != 0;
> }
> 
> mn10300: 0/1
> 
> static inline int test_bit(unsigned long nr, const volatile void *addr)
> {
>         return 1UL & (((const volatile unsigned int *) addr)[nr >> 5] >> (nr & 31));
> }
> 
> s390: 0/1
> 
> static inline int test_bit(unsigned long nr, const volatile unsigned long *ptr)
> {
>         const volatile unsigned char *addr;
> 
>         addr = ((const volatile unsigned char *)ptr);
>         addr += (nr ^ (BITS_PER_LONG - 8)) >> 3;
>         return (*addr >> (nr & 7)) & 1;
> }
> 
> x86: 0/1 for constant, ? for variable
> 
> static __always_inline int constant_test_bit(long nr, const volatile unsigned long *addr)
> {
>         return ((1UL << (nr & (BITS_PER_LONG-1))) &
>                 (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
> }
> (presumably variable_test_bit is the same, but I don't know)
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>

-next regression: "driver cohandle -EPROBE_DEFER from bus_type.match()"

[Marek Szyprowski]

Hello,

On 2015-12-17 19:46, Russell King - ARM Linux wrote:
> On Thu, Dec 17, 2015 at 07:51:14AM -0800, Dan Williams wrote:
>> The commit below causes the libnvdimm sub-system to stop loading.
>> This is due to the fact that nvdimm_bus_match() returns the result of
>> test_bit() which may be negative.  If there are any other bus match
>> functions using test_bit they may be similarly impacted.
>>
>> Can we queue a fixup like the following to libnvdimm, and maybe
>> others, ahead of this driver core change?
> This is rather annoying.  Have we uncovered a latent bug in other
> architectures?  Well, looking through the test_bit() implementations,
> it looks like it.
>
> I'll drop the patch set for the time being, we can't go around breaking
> stuff like this.  However, I think the test_bit() result should be
> regularised across different architectures - it _looks_ to me like most
> implementations return 0/1 values, but there may be some that don't
> (maybe the assembly versions?)

I've checked all the functions assigned to match entry of struct bus_type
objects and found that there are only 4 such functions that don't return 0/1
values:

1. arch/arm/common/sa1111.c: sa1111_match -> result of bitwise &
2. drivers/nvdimm/bus.c: nvdimm_bus_match -> result of test_bit()
3. drivers/sh/superhyway/superhyway.c: superhyway_bus_match -> error codes
     (this is really funny case, all errors are resolved to 'matched' case)
4. drivers/staging/unisys/visorbus/visorbus_main.c: visorbus_match -> 0 and
     some positive integer values, safe for now

The list of functions that I've examined has been generated by following
command:
$ git grep -Pp "\.match\s" | grep -A1 bus_type | grep "\.match\s" | tr 
-s "\t;&,=:" " " | cut -d" " -f1,3

Only the first two functions require potential fixing to ensure that correct
match will not result in negative return value, so the following 
extension to
the patch proposed by Dan Williams fixes all of them:

From: Marek Szyprowski <m.szyprowski@samsung.com>
Date: Fri, 18 Dec 2015 15:17:12 +0100
Subject: [PATCH] drivers: ensure no negative value gets returned on positive
  match

Commit 09a14906a26e454cad7ff0ad96af40fc4cd90eb0 "ARM: 8472/1: driver
cohandle -EPROBE_DEFER from bus_type.match()") added support for returning
error values from bus_match callback. This patch ensures that all existing
match callbacks don't return negative values (potential errors) in case
of positive match.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
  arch/arm/common/sa1111.c | 2 +-
  drivers/nvdimm/bus.c     | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 3d22494..fb0a0a4 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, 
struct device_driver *_drv)
         struct sa1111_dev *dev = SA1111_DEV(_dev);
         struct sa1111_driver *drv = SA1111_DRV(_drv);

-       return dev->devid & drv->devid;
+       return !!(dev->devid & drv->devid);
  }

  static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..2b2181c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev, struct 
device_driver *drv)
  {
         struct nd_device_driver *nd_drv = to_nd_device_driver(drv);

-       return test_bit(to_nd_device_type(dev), &nd_drv->type);
+       return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
  }

  static struct module *to_bus_provider(struct device *dev)
--
1.9.2


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland