Re: [PATCH v7 3/4] gpiolib: Pass array info to get/set array functions

2018-09-05 Thread kbuild test robot
Hi Janusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.19-rc2 next-20180905]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Janusz-Krzysztofik/gpiolib-Pass-bitmaps-not-integer-arrays-to-get-set-array/20180903-172834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git 
for-next
config: x86_64-randconfig-f2-201835 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/phy/motorola/phy-mapphone-mdm6600.c: In function 'phy_mdm6600_cmd':
   drivers/phy/motorola/phy-mapphone-mdm6600.c:166:12: error: passing argument 
3 of 'gpiod_set_array_value_cansleep' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   ddata->cmd_gpios->info, values);
   ^
   In file included from drivers/phy/motorola/phy-mapphone-mdm6600.c:16:0:
   include/linux/gpio/consumer.h:417:20: note: expected 'int *' but argument is 
of type 'struct gpio_array *'
static inline void gpiod_set_array_value_cansleep(unsigned int array_size,
   ^~
>> drivers/phy/motorola/phy-mapphone-mdm6600.c:164:2: error: too many arguments 
>> to function 'gpiod_set_array_value_cansleep'
 gpiod_set_array_value_cansleep(PHY_MDM6600_NR_CMD_LINES,
 ^~
   In file included from drivers/phy/motorola/phy-mapphone-mdm6600.c:16:0:
   include/linux/gpio/consumer.h:417:20: note: declared here
static inline void gpiod_set_array_value_cansleep(unsigned int array_size,
   ^~
   drivers/phy/motorola/phy-mapphone-mdm6600.c: In function 
'phy_mdm6600_status':
   drivers/phy/motorola/phy-mapphone-mdm6600.c:185:13: error: passing argument 
3 of 'gpiod_get_array_value_cansleep' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
ddata->status_gpios->info,
^
   In file included from drivers/phy/motorola/phy-mapphone-mdm6600.c:16:0:
   include/linux/gpio/consumer.h:404:19: note: expected 'int *' but argument is 
of type 'struct gpio_array *'
static inline int gpiod_get_array_value_cansleep(unsigned int array_size,
  ^~
>> drivers/phy/motorola/phy-mapphone-mdm6600.c:183:10: error: too many 
>> arguments to function 'gpiod_get_array_value_cansleep'
 error = gpiod_get_array_value_cansleep(PHY_MDM6600_NR_STATUS_LINES,
 ^~
   In file included from drivers/phy/motorola/phy-mapphone-mdm6600.c:16:0:
   include/linux/gpio/consumer.h:404:19: note: declared here
static inline int gpiod_get_array_value_cansleep(unsigned int array_size,
  ^~
   cc1: some warnings being treated as errors

vim +/gpiod_set_array_value_cansleep +164 
drivers/phy/motorola/phy-mapphone-mdm6600.c

5d1ebbda0 Tony Lindgren  2018-03-08  151  
5d1ebbda0 Tony Lindgren  2018-03-08  152  /**
5d1ebbda0 Tony Lindgren  2018-03-08  153   * phy_mdm6600_cmd() - send a 
command request to mdm6600
5d1ebbda0 Tony Lindgren  2018-03-08  154   * @ddata: device driver data
5d1ebbda0 Tony Lindgren  2018-03-08  155   *
5d1ebbda0 Tony Lindgren  2018-03-08  156   * Configures the three command 
request GPIOs to the specified value.
5d1ebbda0 Tony Lindgren  2018-03-08  157   */
5d1ebbda0 Tony Lindgren  2018-03-08  158  static void 
phy_mdm6600_cmd(struct phy_mdm6600 *ddata, int val)
5d1ebbda0 Tony Lindgren  2018-03-08  159  {
916010a73 Janusz Krzysztofik 2018-09-02  160DECLARE_BITMAP(values, 
PHY_MDM6600_NR_CMD_LINES);
5d1ebbda0 Tony Lindgren  2018-03-08  161  
916010a73 Janusz Krzysztofik 2018-09-02  162*values = val;
5d1ebbda0 Tony Lindgren  2018-03-08  163  
5d1ebbda0 Tony Lindgren  2018-03-08 @164
gpiod_set_array_value_cansleep(PHY_MDM6600_NR_CMD_LINES,
dea6937cb Janusz Krzysztofik 2018-09-02  165   
ddata->cmd_gpios->desc,
dea6937cb Janusz Krzysztofik 2018-09-02 @166   
ddata->cmd_gpios->info, values);
5d1ebbda0 Tony Lindgren  2018-03-08  167  }
5d1ebbda0 Tony Lindgren  2018-03-08  168  
5d1ebbda0 Tony Lindgren  2018-03-08  169  /**
5d1ebbda0 Tony Lindgren  2018-03-08  170   * phy_mdm6600_status() - read 
mdm6600 status lines
5d1ebbda0 Tony Lindgren  2018-03-08  171   * @ddata: device driver data
5d1ebbda0 Tony Lindgren  2018-03-08  172   */
5d1ebbda0 Tony Lindgren  2018-03-08  173  static void 
phy_m

Re: yield() and cond_resched() do not yield to another thread

2018-09-05 Thread Stephen Hemminger
On Tue, 4 Sep 2018 21:16:07 +0800
fei phung  wrote:

> Hi everyone,
> 
> I am working on https://github.com/promach/riffa/tree/full_duplex
> 
> While I modifying the linux driver, I faced some issue using yield().
> 
> Why riffa_driver.c yield() function
> https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L746
> 
> does not yield to chnl_send() thread
> https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L883
> ?
> 
> See 
> https://stackoverflow.com/questions/52166325/why-yield-and-cond-resched-do-not-yield-to-another-thread
> for more details
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Using yield in modern code is almost always wrong. It has priority inversion 
and latency
issues. You are much better off using a real syncronization primitive (like 
completion or wait).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: yield() and cond_resched() do not yield to another thread

2018-09-05 Thread Stephen Hemminger
On Tue, 4 Sep 2018 21:16:07 +0800
fei phung  wrote:

> Hi everyone,
> 
> I am working on https://github.com/promach/riffa/tree/full_duplex
> 
> While I modifying the linux driver, I faced some issue using yield().
> 
> Why riffa_driver.c yield() function
> https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L746
> 
> does not yield to chnl_send() thread
> https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L883
> ?
> 
> See 
> https://stackoverflow.com/questions/52166325/why-yield-and-cond-resched-do-not-yield-to-another-thread
> for more details
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

2018-09-05 Thread Martijn Coenen
This allows the context manager to retrieve information about nodes
that it holds a reference to, such as the current number of
references to those nodes.

Such information can for example be used to determine whether the
servicemanager is the only process holding a reference to a node.
This information can then be passed on to the process holding the
node, which can in turn decide whether it wants to shut down to
reduce resource usage.

Signed-off-by: Martijn Coenen 
---
 drivers/android/binder.c| 50 +
 include/uapi/linux/android/binder.h |  8 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b0090..1c7e965241fb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4544,6 +4544,37 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
 }
 
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
+   struct binder_node_info_for_ref *info)
+{
+   struct binder_node *node;
+   struct binder_context *context = proc->context;
+   __u32 handle = info->handle;
+
+   memset(info, 0, sizeof(*info));
+
+   /* This ioctl may only be used by the context manager */
+   mutex_lock(&context->context_mgr_node_lock);
+   if (!context->binder_context_mgr_node ||
+   context->binder_context_mgr_node->proc != proc) {
+   mutex_unlock(&context->context_mgr_node_lock);
+   return -EPERM;
+   }
+   mutex_unlock(&context->context_mgr_node_lock);
+
+   node = binder_get_node_from_ref(proc, handle, true, NULL);
+   if (!node)
+   return -EINVAL;
+
+   info->strong_count = node->local_strong_refs +
+   node->internal_strong_refs;
+   info->weak_count = node->local_weak_refs;
+
+   binder_put_node(node);
+
+   return 0;
+}
+
 static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
struct binder_node_debug_info *info)
 {
@@ -4638,6 +4669,25 @@ static long binder_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
}
break;
}
+   case BINDER_GET_NODE_INFO_FOR_REF: {
+   struct binder_node_info_for_ref info;
+
+   if (copy_from_user(&info, ubuf, sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   ret = binder_ioctl_get_node_info_for_ref(proc, &info);
+   if (ret < 0)
+   goto err;
+
+   if (copy_to_user(ubuf, &info, sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   break;
+   }
case BINDER_GET_NODE_DEBUG_INFO: {
struct binder_node_debug_info info;
 
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index bfaec6903b8bc..a54a680ff2936 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -200,6 +200,13 @@ struct binder_node_debug_info {
__u32has_weak_ref;
 };
 
+struct binder_node_info_for_ref {
+   __u32handle;
+   __u32strong_count;
+   __u32weak_count;
+   __u64reserved;
+};
+
 #define BINDER_WRITE_READ  _IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -208,6 +215,7 @@ struct binder_node_debug_info {
 #define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
 #define BINDER_VERSION _IOWR('b', 9, struct binder_version)
 #define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct 
binder_node_debug_info)
+#define BINDER_GET_NODE_INFO_FOR_REF   _IOWR('b', 12, struct 
binder_node_info_for_ref)
 
 /*
  * NOTE: Two special error codes you should check for when calling
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/2] staging: rtl8188eu: remove empty if statement in rtw_led.c

2018-09-05 Thread Dan Carpenter
On Tue, Sep 04, 2018 at 01:16:15PM +0200, Michael Straube wrote:
> Remove empty if statement from 'if - else if' and replace the
> else if with if. Remove the now unused variable pmlmepriv.
> Also clears line over 80 characters and CamelCase checkpatch
> issues.
> 
> Signed-off-by: Michael Straube 
> ---
>  drivers/staging/rtl8188eu/core/rtw_led.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_led.c 
> b/drivers/staging/rtl8188eu/core/rtw_led.c
> index cbef871a7679..39904ab284fd 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_led.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_led.c
> @@ -239,7 +239,6 @@ static void SwLedControlMode1(struct adapter *padapter, 
> enum LED_CTL_MODE LedAct
>  {
>   struct led_priv *ledpriv = &padapter->ledpriv;
>   struct LED_871x *pLed = &ledpriv->SwLed0;
> - struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>  
>   switch (LedAction) {
>   case LED_CTL_POWER_ON:
> @@ -290,9 +289,7 @@ static void SwLedControlMode1(struct adapter *padapter, 
> enum LED_CTL_MODE LedAct
>   }
>   break;
>   case LED_CTL_SITE_SURVEY:
> - if ((pmlmepriv->LinkDetectInfo.bBusyTraffic) && 
> (check_fwstate(pmlmepriv, _FW_LINKED))) {
> - ;
> - } else if (!pLed->bLedScanBlinkInProgress) {
> + if (!pLed->bLedScanBlinkInProgress) {

I think you have introduced a bug...

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

2018-09-05 Thread Dan Carpenter
On Wed, Sep 05, 2018 at 09:33:46AM +0200, Martijn Coenen wrote:
> diff --git a/include/uapi/linux/android/binder.h 
> b/include/uapi/linux/android/binder.h
> index bfaec6903b8bc..a54a680ff2936 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -200,6 +200,13 @@ struct binder_node_debug_info {
>   __u32has_weak_ref;
>  };
>  
> +struct binder_node_info_for_ref {
> + __u32handle;
> + __u32strong_count;
> + __u32weak_count;
> + __u64reserved;
> +};

What's the reserved for?  On 64 bit systems there is a 4 byte struct
hole between weak_count and reserved.  Why not just make reserved a
__u32 and get rid of the hole?  (Not rhetorical, I have no idea).

Btw, people sometimes complain about that we don't check that user input
is zeroed in ioctls.  Like for example maybe they're passing random data
in the the strong_count field and then later we decide that actually
that field should mean something but we can't make it mean anything
because we've been letting the user put whatever they want there.  These
are just random thoughts in my head, not necessarily important.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH] staging: most: video: fix registration of an empty comp core_component

2018-09-05 Thread Colin King
From: Colin Ian King 

Currently we have structrues comp (which is empty) and comp_info being
used to register and deregister the component.  This mismatch in naming
occurred from a previous commit that renamed aim_info to comp. Fix this
to use consistent component naming in line with most/net, most/sound etc.

This fixes the message two issues, one with a null empty name when
loading the module:

[ 1485.269515] most_core: registered new core component (null)

and an Oops when removing the module:

[ 1485.277971] BUG: unable to handle kernel NULL pointer dereference at 
0008
[ 1485.278648] PGD 0 P4D 0
[ 1485.279253] Oops: 0002 [#2] SMP PTI
[ 1485.279847] CPU: 1 PID: 32629 Comm: modprobe Tainted: P  D WC OE 
4.18.0-8-generic #9
[ 1485.280442] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
0.0.0 02/06/2015
[ 1485.281040] RIP: 0010:most_deregister_component+0x3c/0x70 [most_core]
.. etc

Fixes: 1b10a0316e2d ("staging: most: video: remove aim designators")
Signed-off-by: Colin Ian King 
---
 drivers/staging/most/video/video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/most/video/video.c 
b/drivers/staging/most/video/video.c
index cf342eb58e10..ad7e28ab9a4f 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -530,7 +530,7 @@ static int comp_disconnect_channel(struct most_interface 
*iface,
return 0;
 }
 
-static struct core_component comp_info = {
+static struct core_component comp = {
.name = "video",
.probe_channel = comp_probe_channel,
.disconnect_channel = comp_disconnect_channel,
@@ -565,7 +565,7 @@ static void __exit comp_exit(void)
}
spin_unlock_irq(&list_lock);
 
-   most_deregister_component(&comp_info);
+   most_deregister_component(&comp);
BUG_ON(!list_empty(&video_devices));
 }
 
-- 
2.17.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: most: video: fix registration of an empty comp core_component

2018-09-05 Thread Alexander Stein
On Wednesday, September 5, 2018, 11:46:05 AM CEST Colin King wrote:
> From: Colin Ian King 
> 
> Currently we have structrues comp (which is empty) and comp_info being
> used to register and deregister the component.  This mismatch in naming
> occurred from a previous commit that renamed aim_info to comp. Fix this
> to use consistent component naming in line with most/net, most/sound etc.
> 
> This fixes the message two issues, one with a null empty name when
> loading the module:
> 
> [ 1485.269515] most_core: registered new core component (null)
> 
> and an Oops when removing the module:
> 
> [ 1485.277971] BUG: unable to handle kernel NULL pointer dereference at 
> 0008
> [ 1485.278648] PGD 0 P4D 0
> [ 1485.279253] Oops: 0002 [#2] SMP PTI
> [ 1485.279847] CPU: 1 PID: 32629 Comm: modprobe Tainted: P  D WC OE 
> 4.18.0-8-generic #9
> [ 1485.280442] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 0.0.0 02/06/2015
> [ 1485.281040] RIP: 0010:most_deregister_component+0x3c/0x70 [most_core]
> .. etc
> 
> Fixes: 1b10a0316e2d ("staging: most: video: remove aim designators")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/most/video/video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c 
> b/drivers/staging/most/video/video.c
> index cf342eb58e10..ad7e28ab9a4f 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -530,7 +530,7 @@ static int comp_disconnect_channel(struct most_interface 
> *iface,
>   return 0;
>  }
>  
> -static struct core_component comp_info = {
> +static struct core_component comp = {
>   .name = "video",
>   .probe_channel = comp_probe_channel,
>   .disconnect_channel = comp_disconnect_channel,

Doesn't it make more sense to move that variable defintion where currently the 
forward declaration is?
This way you can't have 2 variables accidentally. You will need forward 
declarations for those two functions, but a mismatch here results in a linker 
error rather than a runtime NULL pointer access

Best regards,
Alexander



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] staging: most: video: fix registration of an empty comp core_component

2018-09-05 Thread Colin Ian King
On 05/09/18 11:06, Alexander Stein wrote:
> On Wednesday, September 5, 2018, 11:46:05 AM CEST Colin King wrote:
>> From: Colin Ian King 
>>
>> Currently we have structrues comp (which is empty) and comp_info being
>> used to register and deregister the component.  This mismatch in naming
>> occurred from a previous commit that renamed aim_info to comp. Fix this
>> to use consistent component naming in line with most/net, most/sound etc.
>>
>> This fixes the message two issues, one with a null empty name when
>> loading the module:
>>
>> [ 1485.269515] most_core: registered new core component (null)
>>
>> and an Oops when removing the module:
>>
>> [ 1485.277971] BUG: unable to handle kernel NULL pointer dereference at 
>> 0008
>> [ 1485.278648] PGD 0 P4D 0
>> [ 1485.279253] Oops: 0002 [#2] SMP PTI
>> [ 1485.279847] CPU: 1 PID: 32629 Comm: modprobe Tainted: P  D WC OE 
>> 4.18.0-8-generic #9
>> [ 1485.280442] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
>> 0.0.0 02/06/2015
>> [ 1485.281040] RIP: 0010:most_deregister_component+0x3c/0x70 [most_core]
>> .. etc
>>
>> Fixes: 1b10a0316e2d ("staging: most: video: remove aim designators")
>> Signed-off-by: Colin Ian King 
>> ---
>>  drivers/staging/most/video/video.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/most/video/video.c 
>> b/drivers/staging/most/video/video.c
>> index cf342eb58e10..ad7e28ab9a4f 100644
>> --- a/drivers/staging/most/video/video.c
>> +++ b/drivers/staging/most/video/video.c
>> @@ -530,7 +530,7 @@ static int comp_disconnect_channel(struct most_interface 
>> *iface,
>>  return 0;
>>  }
>>  
>> -static struct core_component comp_info = {
>> +static struct core_component comp = {
>>  .name = "video",
>>  .probe_channel = comp_probe_channel,
>>  .disconnect_channel = comp_disconnect_channel,
> 
> Doesn't it make more sense to move that variable defintion where currently 
> the forward declaration is?

Probably, I was just keeping this the same as the other most/* drivers
to be consistent with those for now while just fixing this current buglet.

> This way you can't have 2 variables accidentally. You will need forward 
> declarations for those two functions, but a mismatch here results in a linker 
> error rather than a runtime NULL pointer access
> 
> Best regards,
> Alexander
> 
> 
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

2018-09-05 Thread Martijn Coenen
On Wed, Sep 5, 2018 at 11:09 AM, Dan Carpenter  wrote:
> What's the reserved for?  On 64 bit systems there is a 4 byte struct
> hole between weak_count and reserved.

There's many more pieces of information that we hold for a node. While
we don't have a use for most of that now, we may want some of it in
the future, and so I thought it would be wise to reserve some space
here so we don't need a new ioctl when that happens. I'm actually not
sure it's common to do things this way.

> Why not just make reserved a
> __u32 and get rid of the hole?  (Not rhetorical, I have no idea).

Because I thought 8 bytes of reserved space would be nice :-) But you
have a good point re:alignment, I should make it two __u32's then.

>
> Btw, people sometimes complain about that we don't check that user input
> is zeroed in ioctls.  Like for example maybe they're passing random data
> in the the strong_count field and then later we decide that actually
> that field should mean something but we can't make it mean anything
> because we've been letting the user put whatever they want there.  These
> are just random thoughts in my head, not necessarily important.

That's a good point, I will change the code to check for that.

Thanks,
Martijn

>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/2] staging: rtl8188eu: remove empty if statement in rtw_led.c

2018-09-05 Thread Michael Straube

On 9/5/18 10:13 AM, Dan Carpenter wrote:

On Tue, Sep 04, 2018 at 01:16:15PM +0200, Michael Straube wrote:


case LED_CTL_SITE_SURVEY:
-   if ((pmlmepriv->LinkDetectInfo.bBusyTraffic) && 
(check_fwstate(pmlmepriv, _FW_LINKED))) {
-   ;
-   } else if (!pLed->bLedScanBlinkInProgress) {
+   if (!pLed->bLedScanBlinkInProgress) {


I think you have introduced a bug...


Ah yes I see now, thanks.
Would it be ok to merge the conditions in a single if?

if ((!pmlmepriv->LinkDetectInfo.bBusyTraffic ||
!check_fwstate(pmlmepriv, _FW_LINKED)) &&
!pLed->bLedScanBlinkInProgress) {


regards,
Michael
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH 1/2] staging: rtl8188eu: remove empty if statement in rtw_led.c

2018-09-05 Thread Dan Carpenter
On Wed, Sep 05, 2018 at 01:44:43PM +0200, Michael Straube wrote:
> On 9/5/18 10:13 AM, Dan Carpenter wrote:
> > On Tue, Sep 04, 2018 at 01:16:15PM +0200, Michael Straube wrote:
> > > 
> > >   case LED_CTL_SITE_SURVEY:
> > > - if ((pmlmepriv->LinkDetectInfo.bBusyTraffic) && 
> > > (check_fwstate(pmlmepriv, _FW_LINKED))) {
> > > - ;
> > > - } else if (!pLed->bLedScanBlinkInProgress) {
> > > + if (!pLed->bLedScanBlinkInProgress) {
> > 
> > I think you have introduced a bug...
> 
> Ah yes I see now, thanks.
> Would it be ok to merge the conditions in a single if?
> 
> if ((!pmlmepriv->LinkDetectInfo.bBusyTraffic ||
> !check_fwstate(pmlmepriv, _FW_LINKED)) &&
  ^
Put an extra space here because it's inside the inner parens.

> !pLed->bLedScanBlinkInProgress) {

So it would be aligned like so:

if ((!pmlmepriv->LinkDetectInfo.bBusyTraffic ||
 !check_fwstate(pmlmepriv, _FW_LINKED)) &&
!pLed->bLedScanBlinkInProgress) {

regards,
dan capenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v8 1/8] media: v4l: Add definitions for MPEG2 slice format and metadata

2018-09-05 Thread Paul Kocialkowski
Hi Hans,

Le lundi 03 septembre 2018 à 10:32 +0200, Hans Verkuil a écrit :
> This looks very nice. I have two more comments, but they can be added
> using a follow-up patch (unless you need a v9 anyway):

I suppose I'll send a v9 to keep things in order here.

And thanks for the review!

> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > Stateless video decoding engines require both the MPEG slices and
> > associated metadata from the video stream in order to decode
> > frames.
> > 
> > This introduces definitions for a new pixel format, describing
> > buffers
> > with MPEG2 slice data, as well as a control structure for passing
> > the
> > frame metadata to drivers.
> > 
> > This is based on work from both Florent Revest and Hugues Fruchet.
> > 
> > Signed-off-by: Paul Kocialkowski 
> > ---
> >  .../media/uapi/v4l/extended-controls.rst  | 177
> > ++
> >  .../media/uapi/v4l/pixfmt-compressed.rst  |  14 ++
> >  .../media/uapi/v4l/vidioc-queryctrl.rst   |  14 +-
> >  .../media/videodev2.h.rst.exceptions  |   2 +
> >  drivers/media/v4l2-core/v4l2-ctrls.c  |  63 +++
> >  drivers/media/v4l2-core/v4l2-ioctl.c  |   1 +
> >  include/media/v4l2-ctrls.h|  18 +-
> >  include/uapi/linux/v4l2-controls.h|  65 +++
> >  include/uapi/linux/videodev2.h|   5 +
> >  9 files changed, 350 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst
> > b/Documentation/media/uapi/v4l/extended-controls.rst
> > index 9f7312bf3365..a9252225b63e 100644
> > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > @@ -1497,6 +1497,183 @@ enum
> > v4l2_mpeg_video_h264_hierarchical_coding_type -
> >  
> >  
> >  
> > +.. _v4l2-mpeg-mpeg2:
> > +
> > +``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
> > +Specifies the slice parameters (as extracted from the
> > bitstream) for the
> > +associated MPEG-2 slice data. This includes the necessary
> > parameters for
> > +configuring a stateless hardware decoding pipeline for MPEG-2.
> > +The bitstream parameters are defined according to the ISO/IEC
> > 13818-2 and
> > +ITU-T Rec. H.262 specifications.
> 
> You need to use references to the specs in the biblio.rst file.
> ISO/IEC 13818-2
> is there already. As far as I can see ISO/IEC 13818-2 == ITU-T Rec.
> H.262, so
> it's only one spec, not two.

Alright, I had missed that. Will do!

> > +
> > +.. c:type:: v4l2_ctrl_mpeg2_slice_params
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table:: struct v4l2_ctrl_mpeg2_slice_params
> > +:header-rows:  0
> > +:stub-columns: 0
> > +:widths:   1 1 2
> > +
> > +* - __u32
> > +  - ``bit_size``
> > +  - Size (in bits) of the current slice data.
> > +* - __u32
> > +  - ``data_bit_offset``
> > +  - Offset (in bits) to the video data in the current slice
> > data.
> > +* - struct :c:type:`v4l2_mpeg2_sequence`
> > +  - ``sequence``
> > +  - Structure with MPEG-2 sequence metadata, merging relevant
> > fields from
> > +   the sequence header and sequence extension parts of the
> > bitstream.
> > +* - struct :c:type:`v4l2_mpeg2_picture`
> > +  - ``picture``
> > +  - Structure with MPEG-2 picture metadata, merging relevant
> > fields from
> > +   the picture header and picture coding extension parts of the
> > bitstream.
> > +* - __u8
> > +  - ``quantiser_scale_code``
> > +  - Code used to determine the quantization scale to use for
> > the IDCT.
> > +* - __u8
> > +  - ``backward_ref_index``
> > +  - Index for the V4L2 buffer to use as backward reference,
> > used with
> > +   B-coded and P-coded frames.
> > +* - __u8
> > +  - ``forward_ref_index``
> > +  - Index for the V4L2 buffer to use as forward reference,
> > used with
> > +   P-coded frames.
> > +
> > +.. c:type:: v4l2_mpeg2_sequence
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table:: struct v4l2_mpeg2_sequence
> > +:header-rows:  0
> > +:stub-columns: 0
> > +:widths:   1 1 2
> > +
> > +* - __u16
> > +  - ``horizontal_size``
> > +  - The width of the displayable part of the frame's luminance
> > component.
> > +* - __u16
> > +  - ``vertical_size``
> > +  - The height of the displayable part of the frame's
> > luminance component.
> > +* - __u32
> > +  - ``vbv_buffer_size``
> > +  - Used to calculate the required size of the video buffering
> > verifier,
> > +   defined (in bits) as: 16 * 1024 * vbv_buffer_size.
> > +* - __u8
> > +  - ``profile_and_level_indication``
> > +  - The current profile and level indication as extracted from
> > the
> > +   bitstream.
> > +* - __u8
> > +  - ``progressive_sequence``
> > +  - Indication that all the frames for the sequence are
> > progressive instead
> > +   of interlaced.
> > +* - __u8

Re: [linux-sunxi] [PATCH v8 3/8] dt-bindings: media: Document bindings for the Cedrus VPU driver

2018-09-05 Thread Paul Kocialkowski
Hi,

Le mardi 28 août 2018 à 16:47 +0200, Corentin Labbe a écrit :
> On Tue, Aug 28, 2018 at 09:34:19AM +0200, Paul Kocialkowski wrote:
> > This adds a device-tree binding document that specifies the
> > properties
> > used by the Cedurs VPU driver, as well as examples.
> 
> typo Cedurs=>Cedrus

Thanks, will be fixed in the next iteration.

Cheers,

Paul

> Regards
-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/


signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

2018-09-05 Thread Paul Kocialkowski
Hi,

Le mardi 28 août 2018 à 22:08 -0300, Ezequiel Garcia a écrit :
> On Tue, 2018-08-28 at 09:34 +0200, Paul Kocialkowski wrote:
> > +static const struct v4l2_m2m_ops cedrus_m2m_ops = {
> > +   .device_run = cedrus_device_run,
> > +   .job_abort  = cedrus_job_abort,
> > +};
> > +
> 
> I think you can get rid of this .job_abort. It should
> simplify your .device_run quite a bit.
> 
> .job_abort is optional now since
> 5525b8314389a0c558d15464e86f438974b94e32.

Alright, I will probably do that for the next revision, since it seems
that there is no particular downside to removing it.

Thanks,

Paul

> Regards,
> Ezequiel

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/


signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

2018-09-05 Thread Paul Kocialkowski
Hi and thanks for the review!

Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > +static int cedrus_request_validate(struct media_request *req)
> > +{
> > +   struct media_request_object *obj, *obj_safe;
> > +   struct v4l2_ctrl_handler *parent_hdl, *hdl;
> > +   struct cedrus_ctx *ctx = NULL;
> > +   struct v4l2_ctrl *ctrl_test;
> > +   unsigned int i;
> > +
> > +   list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> 
> You don't need to use the _safe variant during validation.

Okay, I'll use the regular one then.

> > +   struct vb2_buffer *vb;
> > +
> > +   if (vb2_request_object_is_buffer(obj)) {
> > +   vb = container_of(obj, struct vb2_buffer, req_obj);
> > +   ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > +   break;
> > +   }
> > +   }
> 
> Interesting question: what happens if more than one buffer is queued in the
> request? This is allowed by the request API and in that case the associated
> controls in the request apply to all queued buffers.
> 
> Would this make sense at all for this driver? If not, then you need to
> check here if there is more than one buffer in the request and document in
> the spec that this is not allowed.

Well, our driver was written with the (unformal) assumption that we
only deal with a pair of one output and one capture buffer. So I will
add a check for this at request validation time and document it in the
spec. Should that be part of the MPEG-2 PIXFMT documentation (and
duplicated for future formats we add support for)?

> If it does make sense, then you need to test this.
> 
> Again, this can be corrected in a follow-up patch, unless there will be a
> v9 anyway.

[...]

> > +static int cedrus_probe(struct platform_device *pdev)
> > +{
> > +   struct cedrus_dev *dev;
> > +   struct video_device *vfd;
> > +   int ret;
> > +
> > +   dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > +   if (!dev)
> > +   return -ENOMEM;
> > +
> > +   dev->dev = &pdev->dev;
> > +   dev->pdev = pdev;
> > +
> > +   ret = cedrus_hw_probe(dev);
> > +   if (ret) {
> > +   dev_err(&pdev->dev, "Failed to probe hardware\n");
> > +   return ret;
> > +   }
> > +
> > +   dev->dec_ops[CEDRUS_CODEC_MPEG2] = &cedrus_dec_ops_mpeg2;
> > +
> > +   mutex_init(&dev->dev_mutex);
> > +   spin_lock_init(&dev->irq_lock);
> > +
> > +   ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > +   if (ret) {
> > +   dev_err(&pdev->dev, "Failed to register V4L2 device\n");
> > +   return ret;
> > +   }
> > +
> > +   dev->vfd = cedrus_video_device;
> > +   vfd = &dev->vfd;
> > +   vfd->lock = &dev->dev_mutex;
> > +   vfd->v4l2_dev = &dev->v4l2_dev;
> > +
> > +   ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > +   if (ret) {
> > +   v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
> > +   goto err_v4l2;
> > +   }
> > +
> > +   snprintf(vfd->name, sizeof(vfd->name), "%s", cedrus_video_device.name);
> > +   video_set_drvdata(vfd, dev);
> > +
> > +   v4l2_info(&dev->v4l2_dev,
> > + "Device registered as /dev/video%d\n", vfd->num);
> > +
> > +   dev->m2m_dev = v4l2_m2m_init(&cedrus_m2m_ops);
> 
> This ^^^ initialization...
> 
> > +   if (IS_ERR(dev->m2m_dev)) {
> > +   v4l2_err(&dev->v4l2_dev,
> > +"Failed to initialize V4L2 M2M device\n");
> > +   ret = PTR_ERR(dev->m2m_dev);
> > +
> > +   goto err_video;
> > +   }
> > +
> > +   dev->mdev.dev = &pdev->dev;
> > +   strlcpy(dev->mdev.model, CEDRUS_NAME, sizeof(dev->mdev.model));
> > +
> > +   media_device_init(&dev->mdev);
> > +   dev->mdev.ops = &cedrus_m2m_media_ops;
> > +   dev->v4l2_dev.mdev = &dev->mdev;
> 
> ...and this ^^^ initialization should be done before you start registering 
> devices.
> 
> > +
> > +   ret = v4l2_m2m_register_media_controller(dev->m2m_dev,
> > +   vfd, MEDIA_ENT_F_PROC_VIDEO_DECODER);
> > +   if (ret) {
> > +   v4l2_err(&dev->v4l2_dev,
> > +"Failed to initialize V4L2 M2M media controller\n");
> > +   goto err_m2m;
> > +   }
> 
> ^^^ this one too.
> 
> If you don't do that, then you are registering partially initialized devices,
> which is never a good idea.
> 
> Just move the video_register_device() call to here, just before the
> media_device_register().
> 
> This is worthy of a v9, since this can cause real problems.

Thanks for pointing this out, will fix in the next revision.

[...]

> > +static const u8 intra_quantization_matrix_default[64] = {
> > +   8,  16, 16, 19, 16, 19, 22, 22,
> > +   22, 22, 22, 22, 26, 24, 26, 27,
> > +   27, 27, 26, 26, 26, 26, 27, 27,
> > +   27, 29, 29, 29, 34, 34, 34, 29,
> > +   29, 29, 27, 27, 29, 29, 32, 32,
> > +   34, 34, 37, 38, 37, 35, 35, 34,
> > +   35, 38, 38, 40, 40, 40, 48, 48,
> > +   46, 46, 56, 56, 58, 69, 69, 83
> > +};
> > +
> > +sta

[PATCH v2 1/2] staging: rtl8188eu: remove empty if statement in rtw_led.c

2018-09-05 Thread Michael Straube
Remove emtpy if statement from 'if - else if' by moving
all conditions into a single if. Also clears a line over
80 characters checkpatch warning.

Signed-off-by: Michael Straube 
---
v2: changed patch 1/2 that was wrong.

 drivers/staging/rtl8188eu/core/rtw_led.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_led.c 
b/drivers/staging/rtl8188eu/core/rtw_led.c
index cbef871a7679..a4d10fc5d3bb 100644
--- a/drivers/staging/rtl8188eu/core/rtw_led.c
+++ b/drivers/staging/rtl8188eu/core/rtw_led.c
@@ -290,9 +290,9 @@ static void SwLedControlMode1(struct adapter *padapter, 
enum LED_CTL_MODE LedAct
}
break;
case LED_CTL_SITE_SURVEY:
-   if ((pmlmepriv->LinkDetectInfo.bBusyTraffic) && 
(check_fwstate(pmlmepriv, _FW_LINKED))) {
-   ;
-   } else if (!pLed->bLedScanBlinkInProgress) {
+   if ((!pmlmepriv->LinkDetectInfo.bBusyTraffic ||
+!check_fwstate(pmlmepriv, _FW_LINKED)) &&
+   !pLed->bLedScanBlinkInProgress) {
if (IS_LED_WPS_BLINKING(pLed))
return;
if (pLed->bLedNoLinkBlinkInProgress) {
-- 
2.18.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH v2 2/2] staging: rtl8188eu: remove unnecessary parentheses in rtw_led.c

2018-09-05 Thread Michael Straube
Remove unnecessary parentheses from conditionals.
Also clears 'Alignment should match open parenthesis'
checkpatch issue.

Signed-off-by: Michael Straube 
---
 drivers/staging/rtl8188eu/core/rtw_led.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_led.c 
b/drivers/staging/rtl8188eu/core/rtw_led.c
index a4d10fc5d3bb..18d5e422a769 100644
--- a/drivers/staging/rtl8188eu/core/rtw_led.c
+++ b/drivers/staging/rtl8188eu/core/rtw_led.c
@@ -18,7 +18,7 @@ static void BlinkTimerCallback(struct timer_list *t)
struct LED_871x *pLed = from_timer(pLed, t, BlinkTimer);
struct adapter *padapter = pLed->padapter;
 
-   if ((padapter->bSurpriseRemoved) || (padapter->bDriverStopped))
+   if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
return;
 
schedule_work(&pLed->BlinkWorkItem);
@@ -457,7 +457,7 @@ void BlinkHandler(struct LED_871x *pLed)
 {
struct adapter *padapter = pLed->padapter;
 
-   if ((padapter->bSurpriseRemoved) || (padapter->bDriverStopped))
+   if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
return;
 
SwLedBlink1(pLed);
@@ -465,8 +465,8 @@ void BlinkHandler(struct LED_871x *pLed)
 
 void LedControl8188eu(struct adapter *padapter, enum LED_CTL_MODE LedAction)
 {
-   if ((padapter->bSurpriseRemoved) || (padapter->bDriverStopped) ||
-  (!padapter->hw_init_completed))
+   if (padapter->bSurpriseRemoved || padapter->bDriverStopped ||
+   !padapter->hw_init_completed)
return;
 
if ((padapter->pwrctrlpriv.rf_pwrstate != rf_on &&
-- 
2.18.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH v8 4/4] gpiolib: Implement fast processing path in get/set array

2018-09-05 Thread Janusz Krzysztofik
Certain GPIO descriptor arrays returned by gpio_get_array() may contain
information on direct mapping of array members to pins of a single GPIO
chip in hardware order.  In such cases, bitmaps of values can be passed
directly from/to the chip's .get/set_multiple() callbacks without
wasting time on iterations.

Add respective code to gpiod_get/set_array_bitmap_complex() functions.
Pins not applicable for fast path are processed as before, skipping
over the 'fast' ones.

Cc: Jonathan Corbet 
Signed-off-by: Janusz Krzysztofik 
---
 Documentation/driver-api/gpio/board.rst| 15 ++
 Documentation/driver-api/gpio/consumer.rst |  8 +++
 drivers/gpio/gpiolib.c | 87 --
 3 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/gpio/board.rst 
b/Documentation/driver-api/gpio/board.rst
index 2c112553df84..c66821e033c2 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -193,3 +193,18 @@ And the table can be added to the board code as follows::
 
 The line will be hogged as soon as the gpiochip is created or - in case the
 chip was created earlier - when the hog table is registered.
+
+Arrays of pins
+--
+In addition to requesting pins belonging to a function one by one, a device may
+also request an array of pins assigned to the function.  The way those pins are
+mapped to the device determines if the array qualifies for fast bitmap
+processing.  If yes, a bitmap is passed over get/set array functions directly
+between a caller and a respective .get/set_multiple() callback of a GPIO chip.
+
+In order to qualify for fast bitmap processing, the pin mapping must meet the
+following requirements:
+- it must belong to the same chip as other 'fast' pins of the function,
+- its index within the function must match its hardware number within the chip.
+
+Open drain and open source pins are excluded from fast bitmap output 
processing.
diff --git a/Documentation/driver-api/gpio/consumer.rst 
b/Documentation/driver-api/gpio/consumer.rst
index 0afd95a12b10..cf992e5ab976 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -388,6 +388,14 @@ array_info should be set to NULL.
 Note that for optimal performance GPIOs belonging to the same chip should be
 contiguous within the array of descriptors.
 
+Still better performance may be achieved if array indexes of the descriptors
+match hardware pin numbers of a single chip.  If an array passed to a get/set
+array function matches the one obtained from gpiod_get_array() and array_info
+associated with the array is also passed, the function may take a fast bitmap
+processing path, passing the value_bitmap argument directly to the respective
+.get/set_multiple() callback of the chip.  That allows for utilization of GPIO
+banks as data I/O ports without much loss of performance.
+
 The return value of gpiod_get_array_value() and its variants is 0 on success
 or negative on error. Note the difference to gpiod_get_value(), which returns
 0 or 1 on success to convey the GPIO value. With the array functions, the GPIO
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd7c1deed132..d7532aa6c0bc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2789,7 +2789,36 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  struct gpio_array *array_info,
  unsigned long *value_bitmap)
 {
-   int i = 0;
+   int err, i = 0;
+
+   /*
+* Validate array_info against desc_array and its size.
+* It should immediately follow desc_array if both
+* have been obtained from the same gpiod_get_array() call.
+*/
+   if (array_info && array_info->desc == desc_array &&
+   array_size <= array_info->size &&
+   (void *)array_info == desc_array + array_info->size) {
+   if (!can_sleep)
+   WARN_ON(array_info->chip->can_sleep);
+
+   err = gpio_chip_get_multiple(array_info->chip,
+array_info->get_mask,
+value_bitmap);
+   if (err)
+   return err;
+
+   if (!raw && !bitmap_empty(array_info->invert_mask, array_size))
+   bitmap_xor(value_bitmap, value_bitmap,
+  array_info->invert_mask, array_size);
+
+   if (bitmap_full(array_info->get_mask, array_size))
+   return 0;
+
+   i = find_first_zero_bit(array_info->get_mask, array_size);
+   } else {
+   array_info = NULL;
+   }
 
while (i < array_size) {
struct gpio_chip *chip = desc_array[i]->gdev->chip;
@@ -2820,7 +2849,12 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,

[PATCH v8 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array

2018-09-05 Thread Janusz Krzysztofik
Most users of get/set array functions iterate consecutive bits of data,
usually a single integer, while processing array of results obtained
from, or building an array of values to be passed to those functions.
Save time wasted on those iterations by changing the functions' API to
accept bitmaps.

All current users are updated as well.

More benefits from the change are expected as soon as planned support
for accepting/passing those bitmaps directly from/to respective GPIO
chip callbacks if applicable is implemented.

Cc: Jonathan Corbet 
Cc: Miguel Ojeda Sandonis 
Cc: Sebastien Bourdelin 
Cc: Lukas Wunner 
Cc: Peter Korsgaard 
Cc: Peter Rosin 
Cc: Andrew Lunn 
Cc: Florian Fainelli 
Cc: "David S. Miller" 
Cc: Rojhalat Ibrahim 
Cc: Dominik Brodowski 
Cc: Russell King 
Cc: Kishon Vijay Abraham I 
Cc: Tony Lindgren 
Cc: Lars-Peter Clausen 
Cc: Michael Hennerich 
Cc: Jonathan Cameron 
Cc: Hartmut Knaack 
Cc: Peter Meerwald-Stadler 
Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: Yegor Yefremov 
Cc: Uwe Kleine-König 
Signed-off-by: Janusz Krzysztofik 
Acked-by: Ulf Hansson 
Reviewed-by: Geert Uytterhoeven 
Tested-by: Geert Uytterhoeven 
---
 Documentation/driver-api/gpio/consumer.rst  | 22 
 drivers/auxdisplay/hd44780.c| 59 +++--
 drivers/bus/ts-nbus.c   | 15 ++
 drivers/gpio/gpio-max3191x.c| 10 ++--
 drivers/gpio/gpiolib.c  | 82 +++--
 drivers/gpio/gpiolib.h  |  4 +-
 drivers/i2c/muxes/i2c-mux-gpio.c| 13 ++---
 drivers/mmc/core/pwrseq_simple.c| 13 ++---
 drivers/mux/gpio.c  | 13 ++---
 drivers/net/phy/mdio-mux-gpio.c | 11 ++--
 drivers/pcmcia/soc_common.c |  7 +--
 drivers/phy/motorola/phy-mapphone-mdm6600.c | 15 +++---
 drivers/staging/iio/adc/ad7606.c|  9 ++--
 drivers/tty/serial/serial_mctrl_gpio.c  |  7 +--
 include/linux/gpio/consumer.h   | 34 ++--
 15 files changed, 137 insertions(+), 177 deletions(-)

diff --git a/Documentation/driver-api/gpio/consumer.rst 
b/Documentation/driver-api/gpio/consumer.rst
index aa03f389d41d..ed68042ddccf 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -323,29 +323,29 @@ The following functions get or set the values of an array 
of GPIOs::
 
int gpiod_get_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
- int *value_array);
+ unsigned long *value_bitmap);
int gpiod_get_raw_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
- int *value_array);
+ unsigned long *value_bitmap);
int gpiod_get_array_value_cansleep(unsigned int array_size,
   struct gpio_desc **desc_array,
-  int *value_array);
+  unsigned long *value_bitmap);
int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
   struct gpio_desc **desc_array,
-  int *value_array);
+  unsigned long *value_bitmap);
 
void gpiod_set_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
-  int *value_array)
+  unsigned long *value_bitmap)
void gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
-  int *value_array)
+  unsigned long *value_bitmap)
void gpiod_set_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
-   int *value_array)
+   unsigned long *value_bitmap)
void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
-   int *value_array)
+   unsigned long *value_bitmap)
 
 The array can be an arbitrary set of GPIOs. The functions will try to access
 GPIOs belonging to the same bank or chip simultaneously if supported by the
@@ -356,8 +356,8 @@ accessed sequentially.
 The functions take three arguments:
* array_size- the number of array elements
* desc_array- an array of GPIO descriptors
-   * value_array   - an array to store the GPIOs' values (get) or
-

[PATCH v8 0/4] gpiolib: speed up GPIO array processing

2018-09-05 Thread Janusz Krzysztofik


The goal is to boost performance of get/set array functions while
processing GPIO arrays which represent pins of a signle chip in
hardware order.  If resulting performance is close to PIO, GPIO API
can be used for data I/O without much loss of speed.

Created and tested on a low end Amstrad Delta board with NAND driver
updated to use GPIO API for data I/O.  Performance degrade compared to
PIO is much better than before the optimization though not quite
satisfactory on my test hardware.


Janusz Krzysztofik (4):
  gpiolib: Pass bitmaps, not integer arrays, to get/set array
  gpiolib: Identify arrays matching GPIO hardware
  gpiolib: Pass array info to get/set array functions
  gpiolib: Implement fast processing path in get/set array


Changelog:
v8:
[PATCH v8 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array:
- replace *values with values[0] where applicable - suggessted by Geert
  Uytterhoeven, thanks,
- drop !! from 3rd argument of __assign_bit() where applicable - suggested
  by Lukas Wunner, thanks,
drivers/gpio/gpiolib.c:
- s/bitnap/bitmap/ - catched by Lukas Wunner, thanks,
drivers/phy/motorola/phy-mapphone-mdm6600.c:
- assign ddata->status directly from bitmap, not an intermediate 'val'
  variable, now used only for debugging purposes,
include/linux/gpio/consumer.h:
- also update API of inline function replacements used if CONFIG_GPIOLIB
  is not defined - catched by kbuild test robot,
[PATCH v8 3/4] gpiolib: Pass array info to get/set array functions:
commit message:
- s/bulids/builds/ - catched by Geert Uytterhoeven, thanks,
drivers/gpio/gpiolib.c:
- add information on array_info arguments to array function descriptions -
  catched by kbuild test robot,
include/linux/gpio/consumer.h:
- also update API of inline function replacements used if CONFIG_GPIOLIB
  is not defined - catched by kbuild test robot.

v7:
- add more people to Cc: - authors and/or those who contributed most to
  the drivers in scope of the change,
[PATCH v7 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set:
- avoid VLAs, use data source type bit number as bitmap size if not
  constant - great recommendation by Peter Rosin, thanks,
- revert names of local variables declared with DECLARE_BITMAP() from
  'value_bitmap' to original names of value arrays they replace (but not
  'value_array') - inspired by Peter Rosin suggestion - thanks!
drivers/gpio/gpio-max3191x.c:
- use bitmap_alloc() to be more consistent with DECLARE_BITMAP() pattern
  used by other consumers,
drivers/phy/motorola/phy-mapphone-mdm6600.c:
- no need to mask unused bits of val before its assignment to bitmap,
  passing PHY_MDM6600_NR_CMD_LINES to gpiod_set_array_value() as array/
  bitmap size provides sufficient protection.

v6:
[PATCH v6 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set
- use DECLARE_BITMAP() macro for declaring value_bitmap - great idea by
  David Laight, thanks!
drivers/auxdisplay/hd44780.c:
- simplify the code and adjust comments as recommended by Geert
  Uytterhoeven - thanks!,
drivers/i2c/muxes/i2c-mux-gpio.c:
- drop .values member of struct gpiomux - details provided by Peter
  Rosin, thanks!, 
drivers/mux/gpio.c:
- drop .val member of struct mux_gpio - details provided by Peter
  Rosin, thanks!,
drivers/net/phy/mdio-mux-gpio.c:
- drop .values member of struct mdio_mux_gpio_state and its processsing.

v5:
[PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set
- drivers/i2c/muxes/i2c-mux-gpio.c:
  - drop assigment of values to struct gpiomux.values, as recommended
by Peter Rosin - thanks!,
  - mark the .values member of the structure as obsolete,
- drivers/mux/gpio.c:
  - drop assigment of values to struct mux_gpio.val, also recommended
by Peter Rosin - thanks!,
  - merk the .val member of the structure as obsolete,
- drivers/auxdisplay/hd44780.c:
  - fix incorrect bitmap size,
  - use >>= operator to simplify notation,
  both catched by Miguel Ojeda - thanks!,
- add Cc: clauses as well as Acked-by: collected so far.
[PATCH v5 2/4] gpiolib: Identify arrays matching GPIO hardware
- add Cc: clause.
[PATCH v5 3/4] gpiolib: Pass array info to get/set array functions
- add Cc: clauses as well as Acked-by: collected so far.
[PATCH v5 4/4] gpiolib: Implement fast processing path in get/set
- add Cc: clause.

v4:
That series was a follow up of the former "mtd: rawnand: ams-delta: Use
gpio-omap accessors for data I/O" which already contained some changes
to gpiolib.  Those previous attempts were commented by Borris Brezillon
who suggested using GPIO API modified to accept bitmaps, and by Linus
Walleij who suggested still more great ideas for further immprovement
of the proposed API changes - thanks!


diffstat:
 Documentation/driver-api/gpio/board.rst |   15 +
 Documentation/driver-api/gpio/consumer.rst  |   48 +++-
 drivers/auxdisplay/hd44780.c|   67 ++
 drivers/bus/ts-nbus.c   |   20 -
 drivers/gpio/gpio-max3191x.c|  

[PATCH v8 2/4] gpiolib: Identify arrays matching GPIO hardware

2018-09-05 Thread Janusz Krzysztofik
Certain GPIO array lookup results may map directly to GPIO pins of a
single GPIO chip in hardware order.  If that condition is recognized
and handled efficiently, significant performance gain of get/set array
functions may be possible.

While processing a request for an array of GPIO descriptors, identify
those which represent corresponding pins of a single GPIO chip.  Skip
over pins which require open source or open drain special processing.
Moreover, identify pins which require inversion.  Pass a pointer to
that information with the array to the caller so it can benefit from
enhanced performance as soon as get/set array functions can accept and
make efficient use of it.

Cc: Jonathan Corbet 
Signed-off-by: Janusz Krzysztofik 
---
 Documentation/driver-api/gpio/consumer.rst |  4 +-
 drivers/gpio/gpiolib.c | 72 +-
 drivers/gpio/gpiolib.h |  9 
 include/linux/gpio/consumer.h  |  9 
 4 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/gpio/consumer.rst 
b/Documentation/driver-api/gpio/consumer.rst
index ed68042ddccf..7e0298b9a7b9 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -109,9 +109,11 @@ For a function using multiple GPIOs all of those can be 
obtained with one call::
   enum gpiod_flags flags)
 
 This function returns a struct gpio_descs which contains an array of
-descriptors::
+descriptors.  It also contains a pointer to a gpiolib private structure which,
+if passed back to get/set array functions, may speed up I/O proocessing::
 
struct gpio_descs {
+   struct gpio_array *info;
unsigned int ndescs;
struct gpio_desc *desc[];
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b66b2191c5c5..141f39308a53 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4174,7 +4174,9 @@ struct gpio_descs *__must_check gpiod_get_array(struct 
device *dev,
 {
struct gpio_desc *desc;
struct gpio_descs *descs;
-   int count;
+   struct gpio_array *array_info = NULL;
+   struct gpio_chip *chip;
+   int count, bitmap_size;
 
count = gpiod_count(dev, con_id);
if (count < 0)
@@ -4190,9 +4192,77 @@ struct gpio_descs *__must_check gpiod_get_array(struct 
device *dev,
gpiod_put_array(descs);
return ERR_CAST(desc);
}
+
descs->desc[descs->ndescs] = desc;
+
+   chip = gpiod_to_chip(desc);
+   /*
+* Select a chip of first array member
+* whose index matches its pin hardware number
+* as a candidate for fast bitmap processing.
+*/
+   if (!array_info && gpio_chip_hwgpio(desc) == descs->ndescs) {
+   struct gpio_descs *array;
+
+   bitmap_size = BITS_TO_LONGS(chip->ngpio > count ?
+   chip->ngpio : count);
+
+   array = kzalloc(struct_size(descs, desc, count) +
+   struct_size(array_info, invert_mask,
+   3 * bitmap_size), GFP_KERNEL);
+   if (!array) {
+   gpiod_put_array(descs);
+   return ERR_PTR(-ENOMEM);
+   }
+
+   memcpy(array, descs,
+  struct_size(descs, desc, descs->ndescs + 1));
+   kfree(descs);
+
+   descs = array;
+   array_info = (void *)(descs->desc + count);
+   array_info->get_mask = array_info->invert_mask +
+ bitmap_size;
+   array_info->set_mask = array_info->get_mask +
+ bitmap_size;
+
+   array_info->desc = descs->desc;
+   array_info->size = count;
+   array_info->chip = chip;
+   bitmap_set(array_info->get_mask, descs->ndescs,
+  count - descs->ndescs);
+   bitmap_set(array_info->set_mask, descs->ndescs,
+  count - descs->ndescs);
+   descs->info = array_info;
+   }
+   /*
+* Unmark members which don't qualify for fast bitmap
+* processing (different chip, not in hardware order)
+*/
+   if (array_info && (chip != array_info->chip ||
+   gpio_chip_hwgpio(desc) != descs->ndescs)) {
+   __clear_bit(descs->ndescs, array_info->get_mask);
+   __cle

[PATCH v8 3/4] gpiolib: Pass array info to get/set array functions

2018-09-05 Thread Janusz Krzysztofik
In order to make use of array info obtained from gpiod_get_array() and
speed up processing of arrays matching single GPIO chip layout, that
information must be passed to get/set array functions.  Extend the
functions' API with that additional parameter and update all users.
Pass NULL if a user builds an array itself from single GPIOs.

Cc: Jonathan Corbet 
Cc: Miguel Ojeda Sandonis 
Cc: Geert Uytterhoeven 
Cc: Sebastien Bourdelin 
Cc: Lukas Wunner 
Cc: Peter Korsgaard 
Cc: Peter Rosin 
Cc: Andrew Lunn 
Cc: Florian Fainelli 
Cc: "David S. Miller" 
Cc: Rojhalat Ibrahim 
Cc: Dominik Brodowski 
Cc: Russell King 
Cc: Kishon Vijay Abraham I 
Cc: Tony Lindgren 
Cc: Lars-Peter Clausen 
Cc: Michael Hennerich 
Cc: Jonathan Cameron 
Cc: Hartmut Knaack 
Cc: Peter Meerwald-Stadler 
Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: Yegor Yefremov 
Cc: Uwe Kleine-König 
Signed-off-by: Janusz Krzysztofik 
Acked-by: Ulf Hansson 
---
 Documentation/driver-api/gpio/consumer.rst  | 14 --
 drivers/auxdisplay/hd44780.c|  8 +++---
 drivers/bus/ts-nbus.c   |  5 ++--
 drivers/gpio/gpio-max3191x.c|  6 +++--
 drivers/gpio/gpiolib.c  | 40 +++--
 drivers/gpio/gpiolib.h  |  2 ++
 drivers/i2c/muxes/i2c-mux-gpio.c|  3 ++-
 drivers/mmc/core/pwrseq_simple.c|  2 +-
 drivers/mux/gpio.c  |  3 ++-
 drivers/net/phy/mdio-mux-gpio.c |  2 +-
 drivers/pcmcia/soc_common.c |  2 +-
 drivers/phy/motorola/phy-mapphone-mdm6600.c |  4 ++-
 drivers/staging/iio/adc/ad7606.c|  3 ++-
 drivers/tty/serial/serial_mctrl_gpio.c  |  2 +-
 include/linux/gpio/consumer.h   | 16 
 15 files changed, 86 insertions(+), 26 deletions(-)

diff --git a/Documentation/driver-api/gpio/consumer.rst 
b/Documentation/driver-api/gpio/consumer.rst
index 7e0298b9a7b9..0afd95a12b10 100644
--- a/Documentation/driver-api/gpio/consumer.rst
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -325,28 +325,36 @@ The following functions get or set the values of an array 
of GPIOs::
 
int gpiod_get_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
+ struct gpio_array *array_info,
  unsigned long *value_bitmap);
int gpiod_get_raw_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
+ struct gpio_array *array_info,
  unsigned long *value_bitmap);
int gpiod_get_array_value_cansleep(unsigned int array_size,
   struct gpio_desc **desc_array,
+  struct gpio_array *array_info,
   unsigned long *value_bitmap);
int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
   struct gpio_desc **desc_array,
+  struct gpio_array *array_info,
   unsigned long *value_bitmap);
 
void gpiod_set_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
+  struct gpio_array *array_info,
   unsigned long *value_bitmap)
void gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
+  struct gpio_array *array_info,
   unsigned long *value_bitmap)
void gpiod_set_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
+   struct gpio_array *array_info,
unsigned long *value_bitmap)
void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
+   struct gpio_array *array_info,
unsigned long *value_bitmap)
 
 The array can be an arbitrary set of GPIOs. The functions will try to access
@@ -358,6 +366,7 @@ accessed sequentially.
 The functions take three arguments:
* array_size- the number of array elements
* desc_array- an array of GPIO descriptors
+   * array_info- optional information obtained from gpiod_array_get()
* value_bitmap  - a bitmap to store the GPIOs' values (get) or
  a bitmap of values to assign to the GPIOs (set)
 
@@ -368,12 +377,13 @@ the struct gpio_descs returned by gpiod_get_array()::
 
struct gpio_descs 

Re: [PATCH] Revert "staging: erofs: disable compiling temporarile"

2018-09-05 Thread Stephen Rothwell
Hi all,

On Wed, 29 Aug 2018 09:44:03 +1000 Stephen Rothwell  
wrote:
>
> On Tue, 28 Aug 2018 22:13:02 +0800 Chao Yu  wrote:
> >
> > On 2018/8/28 21:05, Greg Kroah-Hartman wrote:  
> > > On Tue, Aug 28, 2018 at 04:56:43PM +0800, Chao Yu wrote:
> > >>
> > >> On 2018/8/28 14:28, Gao Xiang wrote:
> > >>>
> > >>> On 2018/8/28 13:44, Greg Kroah-Hartman wrote:
> >  On Tue, Aug 28, 2018 at 11:39:48AM +0800, Gao Xiang wrote:
> > > This reverts commit 156c3df8d4db4e693c062978186f44079413d74d.
> > >
> > > Since XArray and the new mount apis aren't merged in 4.19-rc1
> > > merge window, the BROKEN mark can be reverted directly without
> > > any problems.
> > >
> > > Fixes: 156c3df8d4db ("staging: erofs: disable compiling temporarile")
> > > Cc: Matthew Wilcox 
> > > Cc: David Howells 
> > > Reviewed-by: Chao Yu 
> > > Signed-off-by: Gao Xiang 
> > > ---
> > >
> > > Hi Greg,
> > >
> > >  Could you please apply this patch to enable EROFS from 4.19-rc2, 
> > > thanks...
> > >
> > >  p.s. We would like to provide a more stable EROFS when linux-4.19 is 
> > > out,
> > >   and there are also two patchsets (the one is already sent out 
> > > by Chao
> > >   and me, the other is previewing in linux-erofs mailing list and 
> > > it will
> > >   be sent out after gathering enough testdata and feedback from 
> > > community
> > >   and carefully reviewed), could you also please consider 
> > > applying these
> > >   two patchsets in the later 4.19-rc (both >2, or the first 
> > > patchset
> > >   could be in rc2 in advance) if it is convenient to do so, or 
> > > the next
> > >   4.20 is also ok...
> > >
> > >  LINK: 
> > > https://lore.kernel.org/lkml/20180821144937.20555-1-c...@kernel.org/
> > >
> > > https://lore.kernel.org/lkml/1535076160-99466-1-git-send-email-gaoxian...@huawei.com/
> > > 
> > 
> >  I applied those patch sets to my -next branch already, right?  So 
> >  those
> > >>>
> > >>> Yes, Thank you for applying those patches. :)
> > >>>
> >  would be going into 4.20-rc1, it is time now for "bugfixes only" for
> >  4.19-final.
> > 
> >  So perhaps we should just leave it as "BROKEN" for now for 4.19 and add
> >  this to my tree now and let people work on it for the next few months 
> >  in
> > >>
> > >> I'm worry about that once we plan to reenable erofs in next x.xx-rc1, in 
> > >> the
> > >> merge window, if there are any other features change common api or 
> > >> structure in
> > >> vfs/mm/block, but related patch didn't cover erofs, that would make 
> > >> conflict
> > >> with erofs.
> > >>
> > >> So if that happens, we can just reminder them to cover erofs? or we 
> > >> should
> > >> handle this by just delay removing 'BROKEN' state?
> > >>
> > >> Thanks,
> > >>
> >  linux-next so that 4.20 has a solid base to start with?
> > 
> > >>>
> > >>> EROFS is be marked as "BROKEN" just because of conflict with
> > >>> XArray and the new mount apis, as Stephen Rothwell suggested in
> > >>>
> > >>> https://lore.kernel.org/lkml/20180802010705.24a72...@canb.auug.org.au/
> > >>>
> >  It might be easiest for Greg to add the disabling CONFIG_EROFS_FS patch
> >  to the staging tree itself for his first pull request during the merge
> >  window and then send a second pull request (after the vfs and maybe the
> >  Xarray stuff has been merged by Linus) with these patches followed by a
> >  revert of the disabling patch.
> > >>>
> > >>> But these two features was still discussing in the mailing list even at 
> > >>> the
> > >>> last time of 4.19-rc1 merge window. I cannot decide whether they were 
> > >>> eventually
> > >>> get merged in 4.19 or not. But it seems that it is regretful that 
> > >>> linux-4.19
> > >>> is out without XArray and the new mount apis.
> > >>>
> > >>> Therefore, I think EROFS should work for linux-4.19 without any 
> > >>> modification
> > >>> if just revert the BROKEN mark.
> > > 
> > > Ok, you are right, I'll go apply this.
> > > 
> > >>> EROFS works fine with the 4.19-rc1 code except that it has some 
> > >>> __GFP_NOFAIL
> > >>> and BUG_ONs on error handling paths and very rarely race between memory
> > >>> reclaiming and decompression... :( I personally think it is complete 
> > >>> enough
> > >>> for people to test since it is an independent and staging filesystem 
> > >>> driver (no
> > >>> other influence...) Anyway, removing EROFS BROKEN mark at 4.20 is also 
> > >>> ok of course...
> > >>>
> > >>> On the other head, if XArray and the new mount apis is still pending 
> > >>> for 4.20,
> > >>> should EROFS uses the same policy as Stephen suggested? I have no idea 
> > >>> how to do next...
> > > 
> > > As the code is now part of the common tree that everyone works off of,
> > > an

Re: [PATCH] Revert "staging: erofs: disable compiling temporarile"

2018-09-05 Thread Gao Xiang
Hi David, Al,

On 2018/9/6 7:25, Stephen Rothwell wrote:
> Hi all,
> 
> On Wed, 29 Aug 2018 09:44:03 +1000 Stephen Rothwell  
> wrote:
>>
>> On Tue, 28 Aug 2018 22:13:02 +0800 Chao Yu  wrote:
>>>
>>> On 2018/8/28 21:05, Greg Kroah-Hartman wrote:  
 On Tue, Aug 28, 2018 at 04:56:43PM +0800, Chao Yu wrote:
>
> On 2018/8/28 14:28, Gao Xiang wrote:
>>
>> On 2018/8/28 13:44, Greg Kroah-Hartman wrote:
>>> On Tue, Aug 28, 2018 at 11:39:48AM +0800, Gao Xiang wrote:
 This reverts commit 156c3df8d4db4e693c062978186f44079413d74d.

 Since XArray and the new mount apis aren't merged in 4.19-rc1
 merge window, the BROKEN mark can be reverted directly without
 any problems.

 Fixes: 156c3df8d4db ("staging: erofs: disable compiling temporarile")
 Cc: Matthew Wilcox 
 Cc: David Howells 
 Reviewed-by: Chao Yu 
 Signed-off-by: Gao Xiang 
 ---

 Hi Greg,

  Could you please apply this patch to enable EROFS from 4.19-rc2, 
 thanks...

  p.s. We would like to provide a more stable EROFS when linux-4.19 is 
 out,
   and there are also two patchsets (the one is already sent out by 
 Chao
   and me, the other is previewing in linux-erofs mailing list and 
 it will
   be sent out after gathering enough testdata and feedback from 
 community
   and carefully reviewed), could you also please consider applying 
 these
   two patchsets in the later 4.19-rc (both >2, or the first 
 patchset
   could be in rc2 in advance) if it is convenient to do so, or the 
 next
   4.20 is also ok...

  LINK: 
 https://lore.kernel.org/lkml/20180821144937.20555-1-c...@kernel.org/

 https://lore.kernel.org/lkml/1535076160-99466-1-git-send-email-gaoxian...@huawei.com/
 
>>>
>>> I applied those patch sets to my -next branch already, right?  So those 
>>>
>>
>> Yes, Thank you for applying those patches. :)
>>
>>> would be going into 4.20-rc1, it is time now for "bugfixes only" for
>>> 4.19-final.
>>>
>>> So perhaps we should just leave it as "BROKEN" for now for 4.19 and add
>>> this to my tree now and let people work on it for the next few months 
>>> in
>
> I'm worry about that once we plan to reenable erofs in next x.xx-rc1, in 
> the
> merge window, if there are any other features change common api or 
> structure in
> vfs/mm/block, but related patch didn't cover erofs, that would make 
> conflict
> with erofs.
>
> So if that happens, we can just reminder them to cover erofs? or we should
> handle this by just delay removing 'BROKEN' state?
>
> Thanks,
>
>>> linux-next so that 4.20 has a solid base to start with?
>>>
>>
>> EROFS is be marked as "BROKEN" just because of conflict with
>> XArray and the new mount apis, as Stephen Rothwell suggested in
>>
>> https://lore.kernel.org/lkml/20180802010705.24a72...@canb.auug.org.au/
>>
>>> It might be easiest for Greg to add the disabling CONFIG_EROFS_FS patch
>>> to the staging tree itself for his first pull request during the merge
>>> window and then send a second pull request (after the vfs and maybe the
>>> Xarray stuff has been merged by Linus) with these patches followed by a
>>> revert of the disabling patch.
>>
>> But these two features was still discussing in the mailing list even at 
>> the
>> last time of 4.19-rc1 merge window. I cannot decide whether they were 
>> eventually
>> get merged in 4.19 or not. But it seems that it is regretful that 
>> linux-4.19
>> is out without XArray and the new mount apis.
>>
>> Therefore, I think EROFS should work for linux-4.19 without any 
>> modification
>> if just revert the BROKEN mark.

 Ok, you are right, I'll go apply this.
 
>> EROFS works fine with the 4.19-rc1 code except that it has some 
>> __GFP_NOFAIL
>> and BUG_ONs on error handling paths and very rarely race between memory
>> reclaiming and decompression... :( I personally think it is complete 
>> enough
>> for people to test since it is an independent and staging filesystem 
>> driver (no
>> other influence...) Anyway, removing EROFS BROKEN mark at 4.20 is also 
>> ok of course...
>>
>> On the other head, if XArray and the new mount apis is still pending for 
>> 4.20,
>> should EROFS uses the same policy as Stephen suggested? I have no idea 
>> how to do next...

 As the code is now part of the common tree that everyone works off of,
 any filesystem changes that happen will normally cover erofs