Re: [PATCH v7 3/4] gpiolib: Pass array info to get/set array functions
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
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
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.
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
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.
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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"
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"
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