Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 9:43 PM chouryzhou(周威) wrote: > > > > > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it > > > will be a static > > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > > me) with > > > no namespace-ization. You will get the same one in all processes, > > > everything is > > > the same as without this patch. > > > > except, as far as I can tell, binder_init_ns() would never have been > > called on it so the mutex and list heads are not initialized so its > > completely broken. Am I missing something? How do those fields get > > initialized in this case? > > > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > > goto err_init_binder_device_failed; > > } > > > > - return ret; > > + ret = binder_init_ns(_ipc_ns); > > + if (ret) > > + goto err_init_namespace_failed; > > > > + return ret; > > They are initialized here. Ok, This init_ipc_ns is a global declared in msgutil.c if SYSVIPC || POSIX_MQUEUE. This seems kinda hacky, but now I finally see why the dependancy... msgutil.c is the only file we can count on if !IPC_NS && (SYSVIPC || POSIX_MQUEUE). There must be a cleaner way to do this, I really don't like this dependency... wouldn't it be cleaner to do: #ifndef CONFIG_IPC_NS static struct ipc_namespace binder_ipc_ns; #define ipcns (_ipc_ns) #else #define ipcns (current->nsproxy->ipc_ns) #endif (and make the initialization of binder_ipc_ns conditional on IPC_NS) This gets us the same thing without the incestuous dependency on the msgutil.c version of init_ipc_ns...and then binder doesn't rely on SYSVIPC or POSIX_MQUEUE directly. > > - choury - > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/android/binder.c: Remove duplicate header
On Fri, Nov 9, 2018 at 8:17 PM Greg KH wrote: > > On Fri, Nov 09, 2018 at 10:40:14PM +0800, kbuild test robot wrote: > > Hi Brajeswar, > > > > Thank you for the patch! Yet something to improve: > > > > [auto build test ERROR on staging/staging-testing] > > [also build test ERROR on v4.20-rc1 next-20181109] > > [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/Brajeswar-Ghosh/drivers-android-binder-c-Remove-duplicate-header/20181109-154717 > > config: i386-allmodconfig (attached as .config) > > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > > reproduce: > > # save the attached .config to linux build tree > > make ARCH=i386 > > > > All errors (new ones prefixed by >>): > > > > Yeah, I was right :( > > Always test-build your patches. Sorry about it. It was a mistake from our side. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > > be a static > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > me) with > > no namespace-ization. You will get the same one in all processes, > > everything is > > the same as without this patch. > > except, as far as I can tell, binder_init_ns() would never have been > called on it so the mutex and list heads are not initialized so its > completely broken. Am I missing something? How do those fields get > initialized in this case? > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > goto err_init_binder_device_failed; > } > > - return ret; > + ret = binder_init_ns(_ipc_ns); > + if (ret) > + goto err_init_namespace_failed; > > + return ret; They are initialized here. - choury - ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 8:43 PM chouryzhou(周威) wrote: > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > be a static > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) > with > no namespace-ization. You will get the same one in all processes, everything > is > the same as without this patch. except, as far as I can tell, binder_init_ns() would never have been called on it so the mutex and list heads are not initialized so its completely broken. Am I missing something? How do those fields get initialized in this case? > > - choury - > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > > > It seems like this mechanism would work even if both are disabled -- > > > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > > > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > > > "#ifndef CONFIG_IPC_NS" > > > > Let me explain it in detail. If SYSIPC and IPC_NS are both defined, > > current->nsproxy->ipc_ns will save the ipc namespace variables. We just use > > it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, > > current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, > > which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set > > (IPC_NS can't be set in this situation), there is no > > current->nsproxy->ipc_ns. > > We make a fack init_ipc_ns here and use it. > > Yes, I can read the code. I'm wondering specifically about SYSVIPC and > POSIX_MQUEUE. Even with your code changes, binder has no dependency on > these configs. Why are you creating one? The actual dependency with > your changes is on "current->nsproxy->ipc_ns" being initialized for > binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it? > > If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this > work? If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will be a static reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) with no namespace-ization. You will get the same one in all processes, everything is the same as without this patch. - choury - ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 7:09 PM chouryzhou(周威) wrote:
>
> >
> > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE.
> > It seems like this mechanism would work even if both are disabled --
> > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and
> > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to
> > "#ifndef CONFIG_IPC_NS"
>
> Let me explain it in detail. If SYSIPC and IPC_NS are both defined,
> current->nsproxy->ipc_ns will save the ipc namespace variables. We just use
> it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set,
> current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c,
> which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set
> (IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns.
> We make a fack init_ipc_ns here and use it.
Yes, I can read the code. I'm wondering specifically about SYSVIPC and
POSIX_MQUEUE. Even with your code changes, binder has no dependency on
these configs. Why are you creating one? The actual dependency with
your changes is on "current->nsproxy->ipc_ns" being initialized for
binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it?
If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this work?
>
> > why eliminate name? The string name is very useful for differentiating
> > normal "framework" binder transactions vs "hal" or "vendor"
> > transactions. If we just have a device number it will be hard to tell
> > in the logs even which namespace it belongs to. We need to keep both
> > the "name" (for which there might be multiple in each ns) and some
> > indication of which namespace this is. Maybe we assign some sort of
> > namespace ID during binder_init_ns().
>
> I will remain the name of device. The inum of ipc_ns can be treated as
> namespace ID in ipc_ns.
>
> > As mentioned above, we need to retain name and probably also want a ns
> > id of some sort. So context now has 3 components if IPC_NS, so maybe a
> > helper function to print context like:
> >
> > static void binder_seq_print_context(struct seq_file *m, struct
> > binder_context *context)
> > {
> > #ifdef CONFIG_IPC_NS
> > seq_printf(m, "%d-%d-%s", context->ns_id, context->device,
> > context->name);
> > #else
> > seq_printf(m, "%d", context->name);
> > #endif
> > }
> >
> > (same comment below everywhere context is printed)
> >
> > Should these debugfs nodes should be ns aware and only print debugging
> > info for the context of the thread accessing the node? If so, we would
> > also want to be namespace-aware when printing pids.
>
> Nowadays, debugfs is not namespace-ized, and pid namespace is not associated
> with ipc namespace. Will it be more complicated to debug this if we just
> print
> the info for current thread? Because we will have to enter the ipc namespace
> firstly. But add ipc inum should be no problem.
With the name and ns id, debugging would be fine from the host-side. I
don't understand the container use cases enough to know if you need to
be able to debug binder transaction failures from within the container
-- in which case it seems like you'd want the container-version of the
PIDs -- but obviously this depends on how the containers are set up
and what the use-cases really are. I'm ok with leaving that for a
later patch.
>
> - choury -
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
>
> I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE.
> It seems like this mechanism would work even if both are disabled --
> as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and
> allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to
> "#ifndef CONFIG_IPC_NS"
Let me explain it in detail. If SYSIPC and IPC_NS are both defined,
current->nsproxy->ipc_ns will save the ipc namespace variables. We just use
it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set,
current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c,
which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set
(IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns.
We make a fack init_ipc_ns here and use it.
> why eliminate name? The string name is very useful for differentiating
> normal "framework" binder transactions vs "hal" or "vendor"
> transactions. If we just have a device number it will be hard to tell
> in the logs even which namespace it belongs to. We need to keep both
> the "name" (for which there might be multiple in each ns) and some
> indication of which namespace this is. Maybe we assign some sort of
> namespace ID during binder_init_ns().
I will remain the name of device. The inum of ipc_ns can be treated as
namespace ID in ipc_ns.
> As mentioned above, we need to retain name and probably also want a ns
> id of some sort. So context now has 3 components if IPC_NS, so maybe a
> helper function to print context like:
>
> static void binder_seq_print_context(struct seq_file *m, struct
> binder_context *context)
> {
> #ifdef CONFIG_IPC_NS
> seq_printf(m, "%d-%d-%s", context->ns_id, context->device,
> context->name);
> #else
> seq_printf(m, "%d", context->name);
> #endif
> }
>
> (same comment below everywhere context is printed)
>
> Should these debugfs nodes should be ns aware and only print debugging
> info for the context of the thread accessing the node? If so, we would
> also want to be namespace-aware when printing pids.
Nowadays, debugfs is not namespace-ized, and pid namespace is not associated
with ipc namespace. Will it be more complicated to debug this if we just print
the info for current thread? Because we will have to enter the ipc namespace
firstly. But add ipc inum should be no problem.
- choury -
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/android/binder.c: Remove duplicate header
Hi Brajeswar,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.20-rc1 next-20181109]
[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/Brajeswar-Ghosh/drivers-android-binder-c-Remove-duplicate-header/20181109-154717
config: i386-randconfig-i3-201844 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/android/binder.o: In function `__read_once_size':
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_return'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_return'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_return'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_buffer_release'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_buffer_release'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_buffer_release'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_wait_for_work'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_fd_recv'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_wait_for_work'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_fd_recv'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_fd_recv'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_wait_for_work'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_received'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_received'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_received'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_alloc_buf'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_failed_buffer_release'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction'
>> include/linux/compiler.h:182: undefined reference to
>> `__tracepoint_binder_transaction_alloc_buf'
vim +182 include/linux/compiler.h
d976441f Andrey Ryabinin 2015-10-19 178
d976441f Andrey Ryabinin 2015-10-19 179 static __always_inline
d976441f Andrey Ryabinin 2015-10-19 180 void __read_once_size(const
volatile void *p, void *res, int size)
230fa253 Christian Borntraeger 2014-11-25 181 {
d976441f Andrey Ryabinin 2015-10-19 @182 __READ_ONCE_SIZE;
230fa253 Christian Borntraeger 2014-11-25 183 }
d976441f Andrey Ryabinin 2015-10-19 184
:: The code at line 182 was first introduced by commit
:: d976441f44bc5d48635d081d277aa76556ffbf8b compiler, atomics, kasan:
Provide READ_ONCE_NOCHECK()
:: TO: Andrey Ryabinin
:: CC: Ingo Molnar
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
On Fri, Nov 9, 2018 at 10:20 PM Greg Kroah-Hartman wrote: > > On Fri, Nov 09, 2018 at 09:19:52PM -0200, Matheus Tavares Bernardino wrote: > > On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam wrote: > > > > > > Hi Matheus, > > > > > > > Hi, Fabio > > > > > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares > > > wrote: > > > > > > > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c, > > > > which solves the checkpatch.pl warning: > > > > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1". > > > > > > > > Signed-off-by: Matheus Tavares > > > > --- > > > > drivers/staging/iio/resolver/ad2s90.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/staging/iio/resolver/ad2s90.c > > > > b/drivers/staging/iio/resolver/ad2s90.c > > > > index 949ff55ac6b0..f439da721df8 100644 > > > > --- a/drivers/staging/iio/resolver/ad2s90.c > > > > +++ b/drivers/staging/iio/resolver/ad2s90.c > > > > @@ -1,3 +1,4 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > > > This should be: > > > // SPDX-License-Identifier: GPL-2.0 > > > > Hm, but it seems that the identifier "GPL-2.0" is deprecated, look: > > https://spdx.org/licenses/GPL-2.0.html. It has been updated to > > "GPL-2.0-only" in license list v3 > > (https://spdx.org/licenses/GPL-2.0-only.html). Is there some other > > reason to use the deprecated "GPL-2.0" that I'm not aware of? > > Yes, please read the in-kernel documentation for all of this at: > Documentation/process/license-rules.rst > > Long story short, we started the adding of these tags to the kernel > before the crazyness of the "-only" markings for GPL in spdx. Let's > keep it this way for now, if we ever get the whole kernel finished, then > we can revisit the markings and maybe do a wholesale conversion, if it's > really needed. > Got it, thanks for the explanation! I'll correct this in v2. Thanks, Matheus > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
On Fri, Nov 09, 2018 at 09:19:52PM -0200, Matheus Tavares Bernardino wrote: > On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam wrote: > > > > Hi Matheus, > > > > Hi, Fabio > > > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares > > wrote: > > > > > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c, > > > which solves the checkpatch.pl warning: > > > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1". > > > > > > Signed-off-by: Matheus Tavares > > > --- > > > drivers/staging/iio/resolver/ad2s90.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/staging/iio/resolver/ad2s90.c > > > b/drivers/staging/iio/resolver/ad2s90.c > > > index 949ff55ac6b0..f439da721df8 100644 > > > --- a/drivers/staging/iio/resolver/ad2s90.c > > > +++ b/drivers/staging/iio/resolver/ad2s90.c > > > @@ -1,3 +1,4 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > This should be: > > // SPDX-License-Identifier: GPL-2.0 > > Hm, but it seems that the identifier "GPL-2.0" is deprecated, look: > https://spdx.org/licenses/GPL-2.0.html. It has been updated to > "GPL-2.0-only" in license list v3 > (https://spdx.org/licenses/GPL-2.0-only.html). Is there some other > reason to use the deprecated "GPL-2.0" that I'm not aware of? Yes, please read the in-kernel documentation for all of this at: Documentation/process/license-rules.rst Long story short, we started the adding of these tags to the kernel before the crazyness of the "-only" markings for GPL in spdx. Let's keep it this way for now, if we ever get the whole kernel finished, then we can revisit the markings and maybe do a wholesale conversion, if it's really needed. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam wrote: > > Hi Matheus, > Hi, Fabio > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares > wrote: > > > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c, > > which solves the checkpatch.pl warning: > > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1". > > > > Signed-off-by: Matheus Tavares > > --- > > drivers/staging/iio/resolver/ad2s90.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/staging/iio/resolver/ad2s90.c > > b/drivers/staging/iio/resolver/ad2s90.c > > index 949ff55ac6b0..f439da721df8 100644 > > --- a/drivers/staging/iio/resolver/ad2s90.c > > +++ b/drivers/staging/iio/resolver/ad2s90.c > > @@ -1,3 +1,4 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > This should be: > // SPDX-License-Identifier: GPL-2.0 Hm, but it seems that the identifier "GPL-2.0" is deprecated, look: https://spdx.org/licenses/GPL-2.0.html. It has been updated to "GPL-2.0-only" in license list v3 (https://spdx.org/licenses/GPL-2.0-only.html). Is there some other reason to use the deprecated "GPL-2.0" that I'm not aware of? Thanks, Matheus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits
Hi
>While I agree that it looks nicer to indent all these to the same level,
>you also need to think about the fact that the kernel git repo is already
>pretty big as-is, so it's a good idea if a patch adds as much code/semantic
>value [as possible] with as little change [as possible] and with as good of
>readability [as possible].
Understood. But can't someone else submit another patch fixing these
indentation issues? That would be another commit and more stuff to the
repository.
On Thu, Nov 8, 2018 at 11:51 AM Ardelean, Alexandru
wrote:
>
> On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > Previously, all pattern_masks and patterns in the chip_info table were
> > hardcoded. Now they are generated using the PAT macros, as described in
> > the datasheets.
>
> One comment about indentation/whitespace.
>
> Rest looks good.
>
> Alex
>
> >
> > Signed-off-by: Giuliano Belinassi
> > ---
> > Changes in v2:
> > - Added the PATTERN and PATTERN_MASK macros
> > - Update BIT macros alignment to match with PATTERN
> > - Generate pattern mask with PATTERN_MASK macros.
> >
> > drivers/staging/iio/adc/ad7780.c | 40 +++-
> > 1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 9ec2b002539e..ff8e3b2d0efc 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -22,14 +22,22 @@
> > #include
> > #include
> >
> > -#define AD7780_RDY BIT(7)
> > -#define AD7780_FILTERBIT(6)
> > -#define AD7780_ERR BIT(5)
> > -#define AD7780_ID1 BIT(4)
> > -#define AD7780_ID0 BIT(3)
> > -#define AD7780_GAIN BIT(2)
> > -#define AD7780_PAT1 BIT(1)
> > -#define AD7780_PAT0 BIT(0)
> > +#define AD7780_RDY BIT(7)
> > +#define AD7780_FILTERBIT(6)
> > +#define AD7780_ERR BIT(5)
> > +#define AD7780_ID1 BIT(4)
> > +#define AD7780_ID0 BIT(3)
> > +#define AD7780_GAIN BIT(2)
> > +#define AD7780_PAT1 BIT(1)
> > +#define AD7780_PAT0 BIT(0)
>
> Changing indentation here doesn't add much value; it's mostly
> patch/whitespace noise.
>
> While I agree that it looks nicer to indent all these to the same level,
> you also need to think about the fact that the kernel git repo is already
> pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> value [as possible] with as little change [as possible] and with as good of
> readability [as possible].
> [ Kind of sounds like a balance act between the 3 things ].
>
>
> > +
> > +#define AD7780_PATTERN (AD7780_PAT0)
> > +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
> > +
> > +#define AD7170_PAT2 BIT(2)
> > +
> > +#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > +#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> >
> > struct ad7780_chip_info {
> > struct iio_chan_specchannel;
> > @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > [ID_AD7170] = {
> > .channel = AD7780_CHANNEL(12, 24),
> > - .pattern = 0x5,
> > - .pattern_mask = 0x7,
> > + .pattern = AD7170_PATTERN,
> > + .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7171] = {
> > .channel = AD7780_CHANNEL(16, 24),
> > - .pattern = 0x5,
> > - .pattern_mask = 0x7,
> > + .pattern = AD7170_PATTERN,
> > + .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7780] = {
> > .channel = AD7780_CHANNEL(24, 32),
> > - .pattern = 0x1,
> > - .pattern_mask = 0x3,
> > + .pattern = AD7780_PATTERN,
> > + .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > [ID_AD7781] = {
> > .channel = AD7780_CHANNEL(20, 32),
> > - .pattern = 0x1,
> > - .pattern_mask = 0x3,
> > + .pattern = AD7780_PATTERN,
> > + .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > };
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-usp+unsubscr...@googlegroups.com.
> To post to this group, send email to kernel-...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kernel-usp/43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel%40analog.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update
> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.
I agree that the patch would be smaller if I do that, but is it really
an issue? If yes, then I will update the patch with this change
> Any reason for setting this explicitly? That's going to be false anyway
It seems clearer to me :-)
On Thu, Nov 8, 2018 at 4:26 PM Tomasz Duszynski wrote:
>
> Hi Giuliano,
>
> Comment inline.
>
> On 11/8/18 2:03 PM, Giuliano Belinassi wrote:
> > Only the ad778x have the 'gain' status bit. Check it before updating
> > through a new variable is_ad778x in chip_info.
> >
> > Signed-off-by: Giuliano Belinassi
> > ---
> > Changes in v2:
> > - Squashed is_ad778x declaration commit with the ad778x checkage
> > - Changed is_ad778x type to bool
> >
> > drivers/staging/iio/adc/ad7780.c | 15 +++
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..9ec2b002539e 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > struct iio_chan_specchannel;
> > unsigned intpattern_mask;
> > unsigned intpattern;
> > + boolis_ad778x;
> > };
> >
> > struct ad7780_state {
> > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > ad_sigma_delta *sigma_delta,
> > ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > return -EIO;
> >
> > - if (raw_sample & AD7780_GAIN)
> > - st->gain = 1;
> > - else
> > - st->gain = 128;
> > + if (chip_info->is_ad778x) {
> > + if (raw_sample & AD7780_GAIN)
> > + st->gain = 1;
> > + else
> > + st->gain = 128;
> > + }
>
> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.
>
> >
> > return 0;
> > }
> > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] = {
> > .channel = AD7780_CHANNEL(12, 24),
> > .pattern = 0x5,
> > .pattern_mask = 0x7,
> > + .is_ad778x = false,
>
> Any reason for setting this explicitly? That's going to be false anyway.
>
> > },
> > [ID_AD7171] = {
> > .channel = AD7780_CHANNEL(16, 24),
> > .pattern = 0x5,
> > .pattern_mask = 0x7,
> > + .is_ad778x = false,
> > },
> > [ID_AD7780] = {
> > .channel = AD7780_CHANNEL(24, 32),
> > .pattern = 0x1,
> > .pattern_mask = 0x3,
> > + .is_ad778x = true,
> > },
> > [ID_AD7781] = {
> > .channel = AD7780_CHANNEL(20, 32),
> > .pattern = 0x1,
> > .pattern_mask = 0x3,
> > + .is_ad778x = true,
> > },
> > };
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-usp+unsubscr...@googlegroups.com.
> To post to this group, send email to kernel-...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kernel-usp/55b5de74-a607-94b9-8c85-40658e38fbb5%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
Hi Matheus, On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares wrote: > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c, > which solves the checkpatch.pl warning: > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1". > > Signed-off-by: Matheus Tavares > --- > drivers/staging/iio/resolver/ad2s90.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/iio/resolver/ad2s90.c > b/drivers/staging/iio/resolver/ad2s90.c > index 949ff55ac6b0..f439da721df8 100644 > --- a/drivers/staging/iio/resolver/ad2s90.c > +++ b/drivers/staging/iio/resolver/ad2s90.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0-only This should be: // SPDX-License-Identifier: GPL-2.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging
This patch set adds device tree support to ad2s90, with standard
device tree id table, adds the respective dt-binding documentation,
solves a codestyle warning and move the driver out of staging.
This patch set completes all the remaining itens listed to be done
before moving the driver out of staging, enumerated in this mail thread:
https://marc.info/?l=linux-iio=154028966111330=2, except by one
codestyle problem: "CHECK: struct mutex definition without comment". It
seems to be a commonly ignored check for mutexes of device states. If I
am wrong, please, let me know and I will be happy to send a patch to
tackle it.
Matheus Tavares (6):
staging:iio:ad2s90: Add device tree support
staging:iio:ad2s90: Remove spi setup that should be done via dt
staging:iio:ad2s90: Add max frequency check at probe
dt-bindings:iio:resolver: Add docs for ad2s90
staging:iio:ad2s90: Add SPDX license identifier
staging:iio:ad2s90: Move out of staging
.../bindings/iio/resolver/ad2s90.txt | 26
drivers/iio/resolver/Kconfig | 10 ++
drivers/iio/resolver/Makefile | 1 +
drivers/{staging => }/iio/resolver/ad2s90.c | 31 ---
drivers/staging/iio/resolver/Kconfig | 10 --
drivers/staging/iio/resolver/Makefile | 1 -
6 files changed, 57 insertions(+), 22 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
rename drivers/{staging => }/iio/resolver/ad2s90.c (81%)
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt
The ad2s90 driver currently sets some spi settings (max_speed_hz and
mode) at ad2s90_probe. This should, instead, be handled via device tree.
This patch removes these configurations from the probe function.
Note: The way in which the mentioned spi settings need to be specified
on the ad2s90's node of a device tree will be documented in the future
patch "dt-bindings:iio:resolver: Add docs for ad2s90".
Signed-off-by: Matheus Tavares
---
drivers/staging/iio/resolver/ad2s90.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/staging/iio/resolver/ad2s90.c
index ff32ca76ca00..95c118c48400 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -77,7 +77,6 @@ static int ad2s90_probe(struct spi_device *spi)
{
struct iio_dev *indio_dev;
struct ad2s90_state *st;
- int ret;
indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
if (!indio_dev)
@@ -94,16 +93,6 @@ static int ad2s90_probe(struct spi_device *spi)
indio_dev->num_channels = 1;
indio_dev->name = spi_get_device_id(spi)->name;
- /* need 600ns between CS and the first falling edge of SCLK */
- spi->max_speed_hz = 83;
- spi->mode = SPI_MODE_3;
- ret = spi_setup(spi);
-
- if (ret < 0) {
- dev_err(>dev, "spi_setup failed!\n");
- return ret;
- }
-
return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
}
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe
This patch adds a max frequency check at the beginning of ad2s90_probe
function so that when it is set to a value above 0.83Mhz, dev_err is
called with an appropriate message and -EINVAL is returned.
The defined limit is 0.83Mhz instead of 2Mhz, which is the chip's max
frequency as specified in the datasheet, because, as also specified in
the datasheet, a 600ns delay is expected between the application of a
logic LO to CS and the application of SCLK. Since the delay is not
implemented in the spi code, to satisfy it, SCLK's period should be at
most 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
gives roughly 83Hz.
Signed-off-by: Matheus Tavares
Signed-off-by: Alexandru Ardelean
---
Alex's S-o-B was added because he gave the code suggestion for this
patch.
drivers/staging/iio/resolver/ad2s90.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/staging/iio/resolver/ad2s90.c
index 95c118c48400..949ff55ac6b0 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -19,6 +19,12 @@
#include
#include
+/*
+ * Although chip's max frequency is 2Mhz, it needs 600ns between CS and the
+ * first falling edge of SCLK, so frequency should be at most 1 / (2 * 6e-7)
+ */
+#define AD2S90_MAX_SPI_FREQ_HZ 83
+
struct ad2s90_state {
struct mutex lock;
struct spi_device *sdev;
@@ -78,6 +84,12 @@ static int ad2s90_probe(struct spi_device *spi)
struct iio_dev *indio_dev;
struct ad2s90_state *st;
+ if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
+ dev_err(>dev, "SPI CLK, %d Hz exceeds %d Hz\n",
+ spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
+ return -EINVAL;
+ }
+
indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 6/6] staging:iio:ad2s90: Move out of staging
Move ad2s90 resolver driver out of staging to the main tree.
Signed-off-by: Matheus Tavares
Signed-off-by: Victor Colombo
---
drivers/iio/resolver/Kconfig| 10 ++
drivers/iio/resolver/Makefile | 1 +
drivers/{staging => }/iio/resolver/ad2s90.c | 0
drivers/staging/iio/resolver/Kconfig| 10 --
drivers/staging/iio/resolver/Makefile | 1 -
5 files changed, 11 insertions(+), 11 deletions(-)
rename drivers/{staging => }/iio/resolver/ad2s90.c (100%)
diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
index 2ced9f22aa70..786801be54f6 100644
--- a/drivers/iio/resolver/Kconfig
+++ b/drivers/iio/resolver/Kconfig
@@ -3,6 +3,16 @@
#
menu "Resolver to digital converters"
+config AD2S90
+ tristate "Analog Devices ad2s90 driver"
+ depends on SPI
+ help
+ Say yes here to build support for Analog Devices spi resolver
+ to digital converters, ad2s90, provides direct access via sysfs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ad2s90.
+
config AD2S1200
tristate "Analog Devices ad2s1200/ad2s1205 driver"
depends on SPI
diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
index 4e1dccae07e7..398d82d50028 100644
--- a/drivers/iio/resolver/Makefile
+++ b/drivers/iio/resolver/Makefile
@@ -2,4 +2,5 @@
# Makefile for Resolver/Synchro drivers
#
+obj-$(CONFIG_AD2S90) += ad2s90.o
obj-$(CONFIG_AD2S1200) += ad2s1200.o
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/iio/resolver/ad2s90.c
similarity index 100%
rename from drivers/staging/iio/resolver/ad2s90.c
rename to drivers/iio/resolver/ad2s90.c
diff --git a/drivers/staging/iio/resolver/Kconfig
b/drivers/staging/iio/resolver/Kconfig
index 6a469ee6101f..4a727c17bb8f 100644
--- a/drivers/staging/iio/resolver/Kconfig
+++ b/drivers/staging/iio/resolver/Kconfig
@@ -3,16 +3,6 @@
#
menu "Resolver to digital converters"
-config AD2S90
- tristate "Analog Devices ad2s90 driver"
- depends on SPI
- help
- Say yes here to build support for Analog Devices spi resolver
- to digital converters, ad2s90, provides direct access via sysfs.
-
- To compile this driver as a module, choose M here: the
- module will be called ad2s90.
-
config AD2S1210
tristate "Analog Devices ad2s1210 driver"
depends on SPI
diff --git a/drivers/staging/iio/resolver/Makefile
b/drivers/staging/iio/resolver/Makefile
index 8d901dc7500b..b2049f2ce36e 100644
--- a/drivers/staging/iio/resolver/Makefile
+++ b/drivers/staging/iio/resolver/Makefile
@@ -2,5 +2,4 @@
# Makefile for Resolver/Synchro drivers
#
-obj-$(CONFIG_AD2S90) += ad2s90.o
obj-$(CONFIG_AD2S1210) += ad2s1210.o
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90
This patch adds the device tree binding documentation for the ad2s90
resolver-to-digital converter.
Signed-off-by: Matheus Tavares
---
.../bindings/iio/resolver/ad2s90.txt | 26 +++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
new file mode 100644
index ..b42cc7752ffd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
@@ -0,0 +1,26 @@
+Analog Devices AD2S90 Resolver-to-Digital Converter
+
+https://www.analog.com/en/products/ad2s90.html
+
+Required properties:
+ - compatible : should be "adi,ad2s90"
+ - reg : SPI chip select number for the device
+ - spi-max-frequency : set maximum clock frequency, must be 83
+ - spi-cpol and spi-cpha : must be defined to enable SPI mode 3
+
+Note about max frequency:
+Chip's max frequency, as specified in its datasheet, is 2Mhz. But a 600ns
+delay is expected between the application of a logic LO to CS and the
+application of SCLK, as also specified. And since the delay is not
+implemented in the spi code, to satisfy it, SCLK's period should be at most
+2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which gives
+roughly 83Hz.
+
+Example:
+resolver@0 {
+ compatible = "adi,ad2s90";
+ reg = <0>;
+ spi-max-frequency = <83>;
+ spi-cpol;
+ spi-cpha;
+};
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c, which solves the checkpatch.pl warning: "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1". Signed-off-by: Matheus Tavares --- drivers/staging/iio/resolver/ad2s90.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c index 949ff55ac6b0..f439da721df8 100644 --- a/drivers/staging/iio/resolver/ad2s90.c +++ b/drivers/staging/iio/resolver/ad2s90.c @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0-only /* * ad2s90.c simple support for the ADI Resolver to Digital Converters: AD2S90 * -- 2.18.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/6] staging:iio:ad2s90: Add device tree support
This patch adds device tree support to ad2s90 with standard
device tree id table.
Signed-off-by: Matheus Tavares
---
drivers/staging/iio/resolver/ad2s90.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/staging/iio/resolver/ad2s90.c
index 3e257ac46f7a..ff32ca76ca00 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -107,6 +107,12 @@ static int ad2s90_probe(struct spi_device *spi)
return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
}
+static const struct of_device_id ad2s90_of_match[] = {
+ { .compatible = "adi,ad2s90", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ad2s90_of_match);
+
static const struct spi_device_id ad2s90_id[] = {
{ "ad2s90" },
{}
@@ -116,6 +122,7 @@ MODULE_DEVICE_TABLE(spi, ad2s90_id);
static struct spi_driver ad2s90_driver = {
.driver = {
.name = "ad2s90",
+ .of_match_table = of_match_ptr(ad2s90_of_match),
},
.probe = ad2s90_probe,
.id_table = ad2s90_id,
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, 09 Nov 2018, Todd Kjos wrote: print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. Ah, very good then. Thanks, Davidlohr ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 10:27 AM Davidlohr Bueso wrote: > > On Thu, 08 Nov 2018, chouryzhou(??) wrote: > > >+#ifdef CONFIG_ANDROID_BINDER_IPC > >+ /* next fields are for binder */ > >+ struct mutex binder_procs_lock; > >+ struct hlist_head binder_procs; > >+ struct hlist_head binder_contexts; > >+#endif > > Now, I took a look at how the binder_procs list is used; and no, what > follows isn't really related to this patch, but a general observation. > > I think that a mutex is also an overkill and you might wanna replace it > with a spinlock/rwlock. Can anything block while holding the > binder_procs_lock? > I don't see anything... you mainly need it for consulting the hlist calling > print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so > no blocking there. print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. > Also, if this is perhaps because of long hold times, dunno, > the rb_first_cached primitive might reduce some of it, although I don't know > how big the rbtrees in binder can get and if it matters at all. > > Anyway, that said and along with addressing Todd's comments, the ipc/ bits > look > good. Feel free to add my: > > Reviewed-by: Davidlohr Bueso > > Thanks, > Davidlohr ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, 08 Nov 2018, chouryzhou(??) wrote: +#ifdef CONFIG_ANDROID_BINDER_IPC + /* next fields are for binder */ + struct mutex binder_procs_lock; + struct hlist_head binder_procs; + struct hlist_head binder_contexts; +#endif Now, I took a look at how the binder_procs list is used; and no, what follows isn't really related to this patch, but a general observation. I think that a mutex is also an overkill and you might wanna replace it with a spinlock/rwlock. Can anything block while holding the binder_procs_lock? I don't see anything... you mainly need it for consulting the hlist calling print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so no blocking there. Also, if this is perhaps because of long hold times, dunno, the rb_first_cached primitive might reduce some of it, although I don't know how big the rbtrees in binder can get and if it matters at all. Anyway, that said and along with addressing Todd's comments, the ipc/ bits look good. Feel free to add my: Reviewed-by: Davidlohr Bueso Thanks, Davidlohr ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v5 3/3] staging: wilc1000: Remove unused mutex cfg_values_lock
From: Adham Abozaeid
After removing cfg_values member, cfg_values_lock that was used to protect
it can also be removed.
Signed-off-by: Adham Abozaeid
---
drivers/staging/wilc1000/host_interface.c | 9 -
drivers/staging/wilc1000/host_interface.h | 2 --
2 files changed, 11 deletions(-)
diff --git a/drivers/staging/wilc1000/host_interface.c
b/drivers/staging/wilc1000/host_interface.c
index 8d1142776310..2bf91df1ded1 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -369,11 +369,8 @@ static void handle_cfg_param(struct work_struct *work)
struct cfg_param_attr *param = >body.cfg_info;
int ret;
struct wid wid_list[32];
- struct host_if_drv *hif_drv = vif->hif_drv;
int i = 0;
- mutex_lock(_drv->cfg_values_lock);
-
if (param->flag & RETRY_SHORT) {
wid_list[i].id = WID_SHORT_RETRY_LIMIT;
wid_list[i].val = (s8 *)>short_retry_limit;
@@ -409,7 +406,6 @@ static void handle_cfg_param(struct work_struct *work)
if (ret)
netdev_err(vif->ndev, "Error in setting CFG params\n");
- mutex_unlock(_drv->cfg_values_lock);
kfree(msg);
}
@@ -3240,15 +3236,10 @@ int wilc_init(struct net_device *dev, struct
host_if_drv **hif_drv_handler)
timer_setup(_drv->connect_timer, timer_connect_cb, 0);
timer_setup(_drv->remain_on_ch_timer, listen_timer_cb, 0);
- mutex_init(_drv->cfg_values_lock);
- mutex_lock(_drv->cfg_values_lock);
-
hif_drv->hif_state = HOST_IF_IDLE;
hif_drv->p2p_timeout = 0;
- mutex_unlock(_drv->cfg_values_lock);
-
wilc->clients_count++;
return 0;
diff --git a/drivers/staging/wilc1000/host_interface.h
b/drivers/staging/wilc1000/host_interface.h
index 1e2e50e91f76..523b46bfb488 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -293,8 +293,6 @@ struct host_if_drv {
enum host_if_state hif_state;
u8 assoc_bssid[ETH_ALEN];
- /*lock to protect concurrent setting of cfg params*/
- struct mutex cfg_values_lock;
struct timer_list scan_timer;
struct wilc_vif *scan_timer_vif;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v5 0/3] staging: wilc1000: validate input to set_wiphy_param and return proper
From: Adham Abozaeid Validate input parameters to set_wiphy_param before scheduling handle_cfg_param() to validate them. This way proper errors can be returned to caller. Also cleaned up unused code in handle_cfg_param. Changes since v1: - Correction spelling in subject of patch#2 - Added From: tag to have correct email from value Changes since v2: - Reverted unnecessary file permission changes Changes since v3: - Fixed 0-day bot warnings for always true conditions for retry_short and retry_long - Fixed typo in From: tag Changes since v4: - Fixed duplicate From: tag Adham Abozaeid (3): staging: wilc1000: validate cfg parameters before scheduling the work staging: wilc1000: Don't keep a copy of wiphy parameters in the driver staging: wilc1000: Remove unused mutex cfg_values_lock drivers/staging/wilc1000/host_interface.c | 75 --- drivers/staging/wilc1000/host_interface.h | 3 - .../staging/wilc1000/wilc_wfi_cfgoperations.c | 32 +++- 3 files changed, 44 insertions(+), 66 deletions(-) -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v5 1/3] staging: wilc1000: validate cfg parameters before scheduling the work
From: Adham Abozaeid
Validate cfg parameters after being called by cfg80211 in set_wiphy_params
before scheduling the work executed in handle_cfg_param
Signed-off-by: Adham Abozaeid
---
drivers/staging/wilc1000/host_interface.c | 61 ++-
.../staging/wilc1000/wilc_wfi_cfgoperations.c | 32 --
2 files changed, 48 insertions(+), 45 deletions(-)
diff --git a/drivers/staging/wilc1000/host_interface.c
b/drivers/staging/wilc1000/host_interface.c
index b89116c57064..c1215c194907 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -377,61 +377,41 @@ static void handle_cfg_param(struct work_struct *work)
if (param->flag & RETRY_SHORT) {
u16 retry_limit = param->short_retry_limit;
- if (retry_limit > 0 && retry_limit < 256) {
- wid_list[i].id = WID_SHORT_RETRY_LIMIT;
- wid_list[i].val = (s8 *)>short_retry_limit;
- wid_list[i].type = WID_SHORT;
- wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.short_retry_limit = retry_limit;
- } else {
- netdev_err(vif->ndev, "Range(1~256) over\n");
- goto unlock;
- }
+ wid_list[i].id = WID_SHORT_RETRY_LIMIT;
+ wid_list[i].val = (s8 *)>short_retry_limit;
+ wid_list[i].type = WID_SHORT;
+ wid_list[i].size = sizeof(u16);
+ hif_drv->cfg_values.short_retry_limit = retry_limit;
i++;
}
if (param->flag & RETRY_LONG) {
u16 limit = param->long_retry_limit;
- if (limit > 0 && limit < 256) {
- wid_list[i].id = WID_LONG_RETRY_LIMIT;
- wid_list[i].val = (s8 *)>long_retry_limit;
- wid_list[i].type = WID_SHORT;
- wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.long_retry_limit = limit;
- } else {
- netdev_err(vif->ndev, "Range(1~256) over\n");
- goto unlock;
- }
+ wid_list[i].id = WID_LONG_RETRY_LIMIT;
+ wid_list[i].val = (s8 *)>long_retry_limit;
+ wid_list[i].type = WID_SHORT;
+ wid_list[i].size = sizeof(u16);
+ hif_drv->cfg_values.long_retry_limit = limit;
i++;
}
if (param->flag & FRAG_THRESHOLD) {
u16 frag_th = param->frag_threshold;
- if (frag_th > 255 && frag_th < 7937) {
- wid_list[i].id = WID_FRAG_THRESHOLD;
- wid_list[i].val = (s8 *)>frag_threshold;
- wid_list[i].type = WID_SHORT;
- wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.frag_threshold = frag_th;
- } else {
- netdev_err(vif->ndev, "Threshold Range fail\n");
- goto unlock;
- }
+ wid_list[i].id = WID_FRAG_THRESHOLD;
+ wid_list[i].val = (s8 *)>frag_threshold;
+ wid_list[i].type = WID_SHORT;
+ wid_list[i].size = sizeof(u16);
+ hif_drv->cfg_values.frag_threshold = frag_th;
i++;
}
if (param->flag & RTS_THRESHOLD) {
u16 rts_th = param->rts_threshold;
- if (rts_th > 255) {
- wid_list[i].id = WID_RTS_THRESHOLD;
- wid_list[i].val = (s8 *)>rts_threshold;
- wid_list[i].type = WID_SHORT;
- wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.rts_threshold = rts_th;
- } else {
- netdev_err(vif->ndev, "Threshold Range fail\n");
- goto unlock;
- }
+ wid_list[i].id = WID_RTS_THRESHOLD;
+ wid_list[i].val = (s8 *)>rts_threshold;
+ wid_list[i].type = WID_SHORT;
+ wid_list[i].size = sizeof(u16);
+ hif_drv->cfg_values.rts_threshold = rts_th;
i++;
}
@@ -441,7 +421,6 @@ static void handle_cfg_param(struct work_struct *work)
if (ret)
netdev_err(vif->ndev, "Error in setting CFG params\n");
-unlock:
mutex_unlock(_drv->cfg_values_lock);
kfree(msg);
}
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 4fd5a64b..a6f4fad43bf7 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1149,21 +1149,45 @@ static int set_wiphy_params(struct wiphy *wiphy, u32
changed)
cfg_param_val.flag = 0;
if (changed &
[PATCH v5 2/3] staging: wilc1000: Don't keep a copy of wiphy parameters in the driver
From: Adham Abozaeid
host_if_drv.cfg_values is a write only member, and can be removed
Signed-off-by: Adham Abozaeid
---
drivers/staging/wilc1000/host_interface.c | 13 -
drivers/staging/wilc1000/host_interface.h | 1 -
2 files changed, 14 deletions(-)
diff --git a/drivers/staging/wilc1000/host_interface.c
b/drivers/staging/wilc1000/host_interface.c
index c1215c194907..8d1142776310 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -375,43 +375,31 @@ static void handle_cfg_param(struct work_struct *work)
mutex_lock(_drv->cfg_values_lock);
if (param->flag & RETRY_SHORT) {
- u16 retry_limit = param->short_retry_limit;
-
wid_list[i].id = WID_SHORT_RETRY_LIMIT;
wid_list[i].val = (s8 *)>short_retry_limit;
wid_list[i].type = WID_SHORT;
wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.short_retry_limit = retry_limit;
i++;
}
if (param->flag & RETRY_LONG) {
- u16 limit = param->long_retry_limit;
-
wid_list[i].id = WID_LONG_RETRY_LIMIT;
wid_list[i].val = (s8 *)>long_retry_limit;
wid_list[i].type = WID_SHORT;
wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.long_retry_limit = limit;
i++;
}
if (param->flag & FRAG_THRESHOLD) {
- u16 frag_th = param->frag_threshold;
-
wid_list[i].id = WID_FRAG_THRESHOLD;
wid_list[i].val = (s8 *)>frag_threshold;
wid_list[i].type = WID_SHORT;
wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.frag_threshold = frag_th;
i++;
}
if (param->flag & RTS_THRESHOLD) {
- u16 rts_th = param->rts_threshold;
-
wid_list[i].id = WID_RTS_THRESHOLD;
wid_list[i].val = (s8 *)>rts_threshold;
wid_list[i].type = WID_SHORT;
wid_list[i].size = sizeof(u16);
- hif_drv->cfg_values.rts_threshold = rts_th;
i++;
}
@@ -3256,7 +3244,6 @@ int wilc_init(struct net_device *dev, struct host_if_drv
**hif_drv_handler)
mutex_lock(_drv->cfg_values_lock);
hif_drv->hif_state = HOST_IF_IDLE;
- hif_drv->cfg_values.scan_source = DEFAULT_SCAN;
hif_drv->p2p_timeout = 0;
diff --git a/drivers/staging/wilc1000/host_interface.h
b/drivers/staging/wilc1000/host_interface.h
index df9fc33be094..1e2e50e91f76 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -293,7 +293,6 @@ struct host_if_drv {
enum host_if_state hif_state;
u8 assoc_bssid[ETH_ALEN];
- struct cfg_param_attr cfg_values;
/*lock to protect concurrent setting of cfg params*/
struct mutex cfg_values_lock;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 8, 2018 at 5:02 AM chouryzhou(周威) wrote:
>
> We are working for running android in container, but we found that binder is
> not isolated by ipc namespace. Since binder is a form of IPC and therefore
> should
> be tied to ipc namespace. With this patch, we can run more than one android
> container on one host.
> This patch move "binder_procs" and "binder_context" into ipc_namespace,
> driver will find the context from it when opening. Although statistics in
> debugfs
> remain global.
>
> Signed-off-by: chouryzhou
> ---
> drivers/android/binder.c | 128
> +++---
> include/linux/ipc_namespace.h | 15 +
> ipc/namespace.c | 10 +++-
> 3 files changed, 117 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cb30a524d16d..22e45bb937e6 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -68,6 +68,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -80,13 +81,18 @@
> #include "binder_alloc.h"
> #include "binder_trace.h"
>
> +
> +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE)
I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE.
It seems like this mechanism would work even if both are disabled --
as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and
allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to
"#ifndef CONFIG_IPC_NS"
> +struct ipc_namespace init_ipc_ns;
> +#define ipcns (_ipc_ns)
> +#else
> +#define ipcns (current->nsproxy->ipc_ns)
> +#endif
> +
> static HLIST_HEAD(binder_deferred_list);
> static DEFINE_MUTEX(binder_deferred_lock);
>
> static HLIST_HEAD(binder_devices);
> -static HLIST_HEAD(binder_procs);
> -static DEFINE_MUTEX(binder_procs_lock);
> -
> static HLIST_HEAD(binder_dead_nodes);
> static DEFINE_SPINLOCK(binder_dead_nodes_lock);
>
> @@ -232,7 +238,7 @@ struct binder_transaction_log_entry {
> int return_error_line;
> uint32_t return_error;
> uint32_t return_error_param;
> - const char *context_name;
> + int context_device;
> };
> struct binder_transaction_log {
> atomic_t cur;
> @@ -263,19 +269,64 @@ static struct binder_transaction_log_entry
> *binder_transaction_log_add(
> }
>
> struct binder_context {
> + struct hlist_node hlist;
> struct binder_node *binder_context_mgr_node;
> struct mutex context_mgr_node_lock;
>
> kuid_t binder_context_mgr_uid;
> - const char *name;
> + intdevice;
> };
>
> struct binder_device {
> struct hlist_node hlist;
> struct miscdevice miscdev;
> - struct binder_context context;
> };
>
> +void binder_exit_ns(struct ipc_namespace *ns)
> +{
> + struct binder_context *context;
> + struct hlist_node *tmp;
> +
> + mutex_destroy(>binder_procs_lock);
> + hlist_for_each_entry_safe(context, tmp, >binder_contexts, hlist) {
> + mutex_destroy(>context_mgr_node_lock);
> + hlist_del(>hlist);
> + kfree(context);
> + }
> +}
> +
> +int binder_init_ns(struct ipc_namespace *ns)
> +{
> + int ret;
> + struct binder_device *device;
> +
> + mutex_init(>binder_procs_lock);
> + INIT_HLIST_HEAD(>binder_procs);
> + INIT_HLIST_HEAD(>binder_contexts);
> +
> + hlist_for_each_entry(device, _devices, hlist) {
> + struct binder_context *context;
> +
> + context = kzalloc(sizeof(*context), GFP_KERNEL);
> + if (!context) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + context->device = device->miscdev.minor;
> + context->binder_context_mgr_uid = INVALID_UID;
> + mutex_init(>context_mgr_node_lock);
> +
> + hlist_add_head(>hlist, >binder_contexts);
> + }
> +
> + return 0;
> +err:
> + binder_exit_ns(ns);
> + return ret;
> +}
> +
> +
> /**
> * struct binder_work - work enqueued on a worklist
> * @entry: node enqueued on list
> @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc,
> e->target_handle = tr->target.handle;
> e->data_size = tr->data_size;
> e->offsets_size = tr->offsets_size;
> - e->context_name = proc->context->name;
> + e->context_device = proc->context->device;
why eliminate name? The string name is very useful for differentiating
normal "framework" binder transactions vs "hal" or "vendor"
transactions. If we just have a device number it will be hard to tell
in the logs even which namespace it belongs to. We need to keep both
the "name" (for which there might be multiple in each ns) and some
indication of which namespace this is. Maybe we assign some sort of
namespace ID during binder_init_ns().
>
> if (reply) {
>
HELP WANTED: Fix -Wmissing-prototypes warnings
Hi all, gcc has a warning option -Wmissing-prototypes which fires when a global function is missing a prototype declaration. This warning is important as it catches cases when that global function's prototype has been changed but its callers haven't been updated. And they should be. Currently, if enabled, this warning triggers ~1400 times for an allmodconfig build and we would like to have 0 warnings and then add that option to the main Makefile. So helping out here would be lovely! And it is very easy to do: you simply build the kernel with W=1: make -j W=1 2>w.log choose one -Wmissing-prototypes warning in the w.log build log and fix it by including the header which has the function prototype. Or you declare the function static. If that function prototype is missing, it needs to be added, of course. For every function, one should ask oneself, is it better to make the function static and save ourselves the prototype and include file dependency - that would be the preferred solution - or if not possible, should one make the prototype visible by including the proper header so that gcc sees the prototype. This should be a good exercise for newbies who'd like to get involved into kernel development. Feel free to ask if there are any questions. Thanks! -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] driver-staging: vsoc.c: Add sysfs support for examining the permissions of regions.
On Wed, Nov 07, 2018 at 10:30:43AM +0800, Jerry Lin wrote:
> Add a attribute called permissions under vsoc device node for examining
> current granted permissions in vsoc_device.
>
> This file will display permissions in following format:
> begin_offset end_offset owner_offset owned_value
> %x %x%x %x
>
(I'm not totally an expert on sysfs rules).
Sysfs are supposed to be one value per file, so instead of doing this
you would create a directory with four files like
vsoc_device/begin_offset vsoc_device/end_offset etc. And each would
just hae a %x output.
> +static ssize_t permissions_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buffer)
> +{
> + struct fd_scoped_permission_node *node;
> + char *row;
> + int ret;
> + ssize_t written = 0;
> +
> + row = kzalloc(sizeof(char) * 128, GFP_KERNEL);
> + if (!row)
> + return 0;
Don't allocate this, just snprintf() directly into the buffer.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix race that allows malicious free of live buffer
On Fri, Nov 9, 2018 at 4:32 AM Greg KH wrote: > > On Tue, Nov 06, 2018 at 03:55:32PM -0800, Todd Kjos wrote: > > Malicious code can attempt to free buffers using the > > BC_FREE_BUFFER ioctl to binder. There are protections > > against a user freeing a buffer while in use by the > > kernel, however there was a window where BC_FREE_BUFFER > > could be used to free a recently allocated buffer that > > was not completely initialized. This resulted in a > > use-after-free detected by KASAN with a malicious > > test program. > > > > This window is closed by setting the buffer's > > allow_user_free attribute to 0 when the buffer > > is allocated or when the user has previously > > freed it instead of waiting for the caller > > to set it. The problem was that when the struct > > buffer was recycled, allow_user_free was stale > > and set to 1 allowing a free to go through. > > > > Signed-off-by: Todd Kjos > > Acked-by: Arve Hjønnevåg > > No "stable" tag here? Any idea how far back the stable backporting > should go, if any? Sorry about that. It should be backported to 4.14 and later. > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: fix undefined LZ4_decompress_safe_partial()
It needs an explicit LZ4 library dependency
if lz4 compression is enabled, found by kbuild randconfig.
Reported-by: kbuild test robot
Fixes: 05f9d4a0c8c4 ("staging: erofs: use the new
LZ4_decompress_safe_partial()")
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
index c8521d7..d04b798 100644
--- a/drivers/staging/erofs/Kconfig
+++ b/drivers/staging/erofs/Kconfig
@@ -90,8 +90,9 @@ config EROFS_FS_IO_MAX_RETRIES
config EROFS_FS_ZIP
bool "EROFS Data Compresssion Support"
depends on EROFS_FS
+ select LZ4_DECOMPRESS
help
- Currently we support VLE Compression only.
+ Currently we support LZ4 VLE Compression only.
Play at your own risk.
If you don't want to use compression feature, say N.
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[staging:staging-testing 95/109] drivers/staging/erofs/unzip_vle_lz4.c:18: undefined reference to `LZ4_decompress_safe_partial'
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
staging-testing
head: 4073536c927421a8908490cf22ce912cb97d7f53
commit: 05f9d4a0c8c439102fb30ff5da53748e807da585 [95/109] staging: erofs: use
the new LZ4_decompress_safe_partial()
config: i386-randconfig-sb0-11092119 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
git checkout 05f9d4a0c8c439102fb30ff5da53748e807da585
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/staging/erofs/unzip_vle_lz4.o: In function `z_erofs_unzip_lz4':
>> drivers/staging/erofs/unzip_vle_lz4.c:18: undefined reference to
>> `LZ4_decompress_safe_partial'
vim +18 drivers/staging/erofs/unzip_vle_lz4.c
15
16 int z_erofs_unzip_lz4(void *in, void *out, size_t inlen, size_t outlen)
17 {
> 18 int ret = LZ4_decompress_safe_partial(in, out, inlen, outlen,
outlen);
19
20 if (ret >= 0)
21 return ret;
22
23 /*
24 * LZ4_decompress_safe_partial will return an error code
25 * (< 0) if decompression failed
26 */
27 errln("%s, failed to decompress, in[%p, %zu] outlen[%p, %zu]",
28__func__, in, inlen, out, outlen);
29 WARN_ON(1);
30 print_hex_dump(KERN_DEBUG, "raw data [in]: ",
DUMP_PREFIX_OFFSET,
31 16, 1, in, inlen, true);
32 print_hex_dump(KERN_DEBUG, "raw data [out]: ",
DUMP_PREFIX_OFFSET,
33 16, 1, out, outlen, true);
34 return -EIO;
35 }
36
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/android/binder.c: Remove duplicate header
On Fri, Nov 09, 2018 at 10:40:14PM +0800, kbuild test robot wrote: > Hi Brajeswar, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on staging/staging-testing] > [also build test ERROR on v4.20-rc1 next-20181109] > [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/Brajeswar-Ghosh/drivers-android-binder-c-Remove-duplicate-header/20181109-154717 > config: i386-allmodconfig (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All errors (new ones prefixed by >>): Yeah, I was right :( Always test-build your patches. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/android/binder.c: Remove duplicate header
Hi Brajeswar, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on v4.20-rc1 next-20181109] [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/Brajeswar-Ghosh/drivers-android-binder-c-Remove-duplicate-header/20181109-154717 config: i386-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/android/binder.o: In function `binder_free_buf': >> binder.c:(.text+0x3c46): undefined reference to >> `__tracepoint_binder_transaction_buffer_release' binder.c:(.text+0x3c91): undefined reference to `__tracepoint_binder_transaction_buffer_release' drivers/android/binder.o: In function `binder_transaction': >> binder.c:(.text+0x4a85): undefined reference to >> `__tracepoint_binder_transaction' binder.c:(.text+0x4af8): undefined reference to `__tracepoint_binder_transaction' >> binder.c:(.text+0x4c33): undefined reference to >> `__tracepoint_binder_transaction_alloc_buf' binder.c:(.text+0x4c90): undefined reference to `__tracepoint_binder_transaction_alloc_buf' >> binder.c:(.text+0x522b): undefined reference to >> `__tracepoint_binder_transaction_node_to_ref' binder.c:(.text+0x5279): undefined reference to `__tracepoint_binder_transaction_node_to_ref' >> binder.c:(.text+0x549b): undefined reference to >> `__tracepoint_binder_transaction_ref_to_node' binder.c:(.text+0x54ee): undefined reference to `__tracepoint_binder_transaction_ref_to_node' >> binder.c:(.text+0x55fd): undefined reference to >> `__tracepoint_binder_transaction_ref_to_ref' binder.c:(.text+0x5655): undefined reference to `__tracepoint_binder_transaction_ref_to_ref' >> binder.c:(.text+0x61c5): undefined reference to >> `__tracepoint_binder_transaction_failed_buffer_release' binder.c:(.text+0x6220): undefined reference to `__tracepoint_binder_transaction_failed_buffer_release' drivers/android/binder.o: In function `binder_thread_write': >> binder.c:(.text+0x6656): undefined reference to `__tracepoint_binder_command' binder.c:(.text+0x66a1): undefined reference to `__tracepoint_binder_command' drivers/android/binder.o: In function `binder_put_node_cmd': >> binder.c:(.text+0x7976): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x79d1): undefined reference to `__tracepoint_binder_return' drivers/android/binder.o: In function `binder_thread_read': >> binder.c:(.text+0x7bf9): undefined reference to >> `__tracepoint_binder_wait_for_work' binder.c:(.text+0x7c71): undefined reference to `__tracepoint_binder_wait_for_work' binder.c:(.text+0x810e): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x8161): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x824e): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x82a1): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x888e): undefined reference to `__tracepoint_binder_return' drivers/android/binder.o:binder.c:(.text+0x88e1): more undefined references to `__tracepoint_binder_return' follow drivers/android/binder.o: In function `binder_thread_read': >> binder.c:(.text+0x8bcf): undefined reference to >> `__tracepoint_binder_transaction_fd_recv' binder.c:(.text+0x8c39): undefined reference to `__tracepoint_binder_transaction_fd_recv' binder.c:(.text+0x8ebe): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x8f11): undefined reference to `__tracepoint_binder_return' >> binder.c:(.text+0x90b2): undefined reference to >> `__tracepoint_binder_transaction_received' binder.c:(.text+0x90fb): undefined reference to `__tracepoint_binder_transaction_received' binder.c:(.text+0x9172): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x91bb): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x947d): undefined reference to `__tracepoint_binder_return' binder.c:(.text+0x94c5): undefined reference to `__tracepoint_binder_return' drivers/android/binder.o: In function `binder_ioctl_write_read.isra.11': >> binder.c:(.text+0x9796): undefined reference to >> `__tracepoint_binder_write_done' binder.c:(.text+0x97e1): undefined reference to `__tracepoint_binder_write_done' >> binder.c:(.text+0x98e6): undefined reference to >> `__tracepoint_binder_read_done' binder.c:(.text+0x9931): undefined reference to `__tracepoint_binder_read_done' drivers/android/binder.o: In function `binder_ioctl': >> binder.c:(.text+0x9ab6): undefined reference to `__tracepoint_binder_ioct
Re: [PATCH] media: staging: tegra-vde: print long unsigned using %lu format specifier
On 11/08/18 12:02, Colin King wrote:
> From: Colin Ian King
>
> The frame.flags & FLAG_B_FRAME is promoted to a long unsigned because
> of the use of the BIT() macro when defining FLAG_B_FRAME and causing a
> build warning. Fix this by using the %lu format specifer.
>
> Cleans up warning:
> drivers/staging/media/tegra-vde/tegra-vde.c:267:5: warning: format
> specifies type 'int' but the argument has type 'unsigned long' [-Wformat]
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/media/tegra-vde/tegra-vde.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
> b/drivers/staging/media/tegra-vde/tegra-vde.c
> index 6f06061a40d9..66cf14212c14 100644
> --- a/drivers/staging/media/tegra-vde/tegra-vde.c
> +++ b/drivers/staging/media/tegra-vde/tegra-vde.c
> @@ -262,7 +262,7 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
> *vde,
> value |= frame->frame_num;
>
> dev_dbg(vde->miscdev.parent,
> - "\tFrame %d: frame_num = %d B_frame = %d\n",
> + "\tFrame %d: frame_num = %d B_frame = %lu\n",
> i + 1, frame->frame_num,
> (frame->flags & FLAG_B_FRAME));
> } else {
>
Compiling for i686 gives:
In file included from
/home/hans/work/build/media-git/include/linux/printk.h:336,
from /home/hans/work/build/media-git/include/linux/kernel.h:14,
from /home/hans/work/build/media-git/include/linux/clk.h:16,
from
/home/hans/work/build/media-git/drivers/staging/media/tegra-vde/tegra-vde.c:12:
/home/hans/work/build/media-git/drivers/staging/media/tegra-vde/tegra-vde.c: In
function 'tegra_vde_setup_iram_tables':
/home/hans/work/build/media-git/drivers/staging/media/tegra-vde/tegra-vde.c:265:5:
warning: format '%lu' expects argument of type 'long unsigned int', but
argument 6 has type 'u32' {aka 'unsigned int'} [-Wformat=]
"\tFrame %d: frame_num = %d B_frame = %lu\n",
^~~~
/home/hans/work/build/media-git/include/linux/dynamic_debug.h:135:39: note: in
definition of macro 'dynamic_dev_dbg'
__dynamic_dev_dbg(, dev, fmt, \
^~~
/home/hans/work/build/media-git/include/linux/device.h:1463:23: note: in
expansion of macro 'dev_fmt'
dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~
/home/hans/work/build/media-git/drivers/staging/media/tegra-vde/tegra-vde.c:264:4:
note: in expansion of macro 'dev_dbg'
dev_dbg(vde->miscdev.parent,
^~~
Should it be %zu?
Regards,
Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix race that allows malicious free of live buffer
On Tue, Nov 06, 2018 at 03:55:32PM -0800, Todd Kjos wrote: > Malicious code can attempt to free buffers using the > BC_FREE_BUFFER ioctl to binder. There are protections > against a user freeing a buffer while in use by the > kernel, however there was a window where BC_FREE_BUFFER > could be used to free a recently allocated buffer that > was not completely initialized. This resulted in a > use-after-free detected by KASAN with a malicious > test program. > > This window is closed by setting the buffer's > allow_user_free attribute to 0 when the buffer > is allocated or when the user has previously > freed it instead of waiting for the caller > to set it. The problem was that when the struct > buffer was recycled, allow_user_free was stale > and set to 1 allowing a free to go through. > > Signed-off-by: Todd Kjos > Acked-by: Arve Hjønnevåg No "stable" tag here? Any idea how far back the stable backporting should go, if any? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: most: use format specifier "%s" in snprintf
From: Colin Ian King
Passing string ch_data_type[i].name as the format specifier is
potentially hazardous because it could (although very unlikely to)
have a format specifier embedded in it causing issues when parsing
the non-existent arguments to these. Follow best practice by using
the "%s" format string for the string.
Cleans up clang warning:
format string is not a string literal (potentially insecure) [-Wformat-security]
Fixes: e7f2b70fd3a9 ("staging: most: replace multiple if..else with table
lookup")
Signed-off-by: Colin Ian King
---
drivers/staging/most/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 6a18cf73c85e..18936cdb1083 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -351,7 +351,7 @@ static ssize_t set_datatype_show(struct device *dev,
for (i = 0; i < ARRAY_SIZE(ch_data_type); i++) {
if (c->cfg.data_type & ch_data_type[i].most_ch_data_type)
- return snprintf(buf, PAGE_SIZE, ch_data_type[i].name);
+ return snprintf(buf, PAGE_SIZE, "%s",
ch_data_type[i].name);
}
return snprintf(buf, PAGE_SIZE, "unconfigured\n");
}
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: How can I get user space tools of erofs?
Hi Chengguang, Good question! It's in the final stage of preparation to open source erofs-mkfs (actually they are now struggle at how to properly spilt into reasonable patches this week), hopefully the implementation could be released at the next week. (sorry I didn't mean to delay, I have to put it in the first place --- successfully launch our linux-erofs products to the market) @Guifu Li is the original author of erofs-mkfs, he will post the original mkfs source code to linux-erofs mailing list and he will maintain erofs-mkfs together with @Wei Fang later. You can contact them for further informations. --- these piece of code is actually not clean enough (a lot hacked/dirty code compared to the kernel code) so a lot of cleanup will be done then. currently, you can get erofs-mkfs binary from (still sorry to say that...): https://github.com/hsiangkao/erofs_mkfs_binary erofs is now in productization for these months, if all things go well, you'll see that HUAWEI mobile phones on the market run in erofs in few months. :) These months I'm busy in solving bugs found by internal beta users and tuning memory policy in heavy memory workload for the best performance compared to ext4 (we have native in-place decompression compared with squashfs/btrfs, thus less extra memory allocation results in lower memory memory reclaim / page-writeback for devices with limited memory, see: https://lists.ozlabs.org/pipermail/linux-erofs/2018-August/000494.html) in order to gain the competitive user experience comparing to uncompressed filesystem solutions. I will update a document to describe our core design and linux-erofs future roadmap in this linux-4.21 round. Thanks, Gao Xiang On 2018/11/9 16:37, cgxu519 wrote: > Hi Xiang, > > Could I ask a simple question about where can I get user space tools(like > mkfs) of erofs? > > > Thanks. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers/android/binder.c: Remove duplicate header
On Fri, Nov 09, 2018 at 09:44:25AM +0530, Brajeswar Ghosh wrote:
> Remove binder_trace.h which is included more than once
>
> Signed-off-by: Brajeswar Ghosh
> ---
> drivers/android/binder.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cb30a524d16d..719f35a5c04b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5852,6 +5852,5 @@ static int __init binder_init(void)
> device_initcall(binder_init);
>
> #define CREATE_TRACE_POINTS
> -#include "binder_trace.h"
>
> MODULE_LICENSE("GPL v2");
Are you sure about this? Did you test the tracepoint functionality
after removing that line? I think you just broke it :(
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface
On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
>
Patch looks good.
I do have some thoughts about it. See inline.
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/adc/ad7816.c | 80 ++--
> 1 file changed, 34 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index bf76a8620bdb..12c4e0ab4713 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -7,7 +7,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -44,9 +44,9 @@
>
> struct ad7816_chip_info {
> struct spi_device *spi_dev;
> - u16 rdwr_pin;
> - u16 convert_pin;
> - u16 busy_pin;
> + struct gpio_desc *rdwr_pin;
> + struct gpio_desc *convert_pin;
> + struct gpio_desc *busy_pin;
Only if you want to do a re-spin, here's an idea.
If you don't want to, feel free to disregard, since your patch is good.
You could compact things a bit; I know this wasn't what the initial code
was doing, but it's an option.
Something like:
enum {
GPIO_RWDR,
GPIO_CONVERT,
GPIO_BUSY,
__GPIO_MAX,
}
Then, what you end up having is:
struct ad7816_chip_info {
struct spi_device *spi_dev;
- u16 rdwr_pin;
- u16 convert_pin;
- u16 busy_pin;
+ struct gpio_desc *gpios[__GPIO_MAX];
// Continued below
> u8 oti_data[AD7816_CS_MAX + 1];
> u8 channel_id; /* 0 always be temperature */
> u8 mode;
> @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> *chip, u16 *data)
> int ret = 0;
> __be16 buf;
>
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
Obviously, in the above context, this becomes:
+ gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
+ gpiod_set_value(chip->gpios[GPIO_RWDR], 0);
> ret = spi_write(spi_dev, >channel_id, sizeof(chip-
> >channel_id));
> if (ret < 0) {
> dev_err(_dev->dev, "SPI channel setting error\n");
> return ret;
> }
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
>
> if (chip->mode == AD7816_PD) { /* operating mode 2 */
> - gpio_set_value(chip->convert_pin, 1);
> - gpio_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
> } else { /* operating mode 1 */
> - gpio_set_value(chip->convert_pin, 0);
> - gpio_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
> }
>
> - while (gpio_get_value(chip->busy_pin))
> + while (gpiod_get_value(chip->busy_pin))
> cpu_relax();
>
> - gpio_set_value(chip->rdwr_pin, 0);
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> ret = spi_read(spi_dev, , sizeof(*data));
> if (ret < 0) {
> dev_err(_dev->dev, "SPI data read error\n");
> @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> *chip, u8 data)
> struct spi_device *spi_dev = chip->spi_dev;
> int ret = 0;
>
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
> ret = spi_write(spi_dev, , sizeof(data));
> if (ret < 0)
> dev_err(_dev->dev, "SPI oti data write error\n");
> @@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device
> *dev,
> struct ad7816_chip_info *chip = iio_priv(indio_dev);
>
> if (strcmp(buf, "full")) {
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
> chip->mode = AD7816_FULL;
> } else {
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 0);
> chip->mode = AD7816_PD;
> }
>
> @@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device *spi_dev)
> {
> struct ad7816_chip_info *chip;
> struct iio_dev *indio_dev;
> - unsigned short *pins = dev_get_platdata(_dev->dev);
> int ret = 0;
> int i;
>
> - if (!pins) {
> - dev_err(_dev->dev, "No necessary GPIO platform
> data.\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(_dev->dev, sizeof(*chip));
> if (!indio_dev)
> return -ENOMEM;
> @@ -364,34 +358,28 @@ static int ad7816_probe(struct spi_device
Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.
On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
One comment inline for this.
Thanks
Alex
>
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/adc/ad7816.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..b8a9149fbac1 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> return 0;
> }
>
> +static const struct of_device_id ad7816_of_match[] = {
> + { .compatible = "adi,ad7816", },
I think you also need to add:
+ { .compatible = "adi,ad7817", },
+ { .compatible = "adi,ad7818", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
> static const struct spi_device_id ad7816_id[] = {
> { "ad7816", ID_AD7816 },
> { "ad7817", ID_AD7817 },
> @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
> static struct spi_driver ad7816_driver = {
> .driver = {
> .name = "ad7816",
> + .of_match_table = of_match_ptr(ad7816_of_match),
> },
> .probe = ad7816_probe,
> .id_table = ad7816_id,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface
On Fri, 2018-11-09 at 08:05 +, Ardelean, Alexandru wrote:
> On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> > instead of the deprecated old non-descriptor interface.
> >
>
> Patch looks good.
> I do have some thoughts about it. See inline.
>
Nevermind what I just said here; I just saw patch3: `iio: ad7816: Set RD/WR
pin and CONVST pin as outputs.`
This looks good as is in the context of the patch series.
>
> > Signed-off-by: Nishad Kamdar
> > ---
> > drivers/staging/iio/adc/ad7816.c | 80 ++--
> > 1 file changed, 34 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7816.c
> > b/drivers/staging/iio/adc/ad7816.c
> > index bf76a8620bdb..12c4e0ab4713 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -7,7 +7,7 @@
> > */
> >
> > #include
> > -#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -44,9 +44,9 @@
> >
> > struct ad7816_chip_info {
> > struct spi_device *spi_dev;
> > - u16 rdwr_pin;
> > - u16 convert_pin;
> > - u16 busy_pin;
> > + struct gpio_desc *rdwr_pin;
> > + struct gpio_desc *convert_pin;
> > + struct gpio_desc *busy_pin;
>
> Only if you want to do a re-spin, here's an idea.
> If you don't want to, feel free to disregard, since your patch is good.
>
> You could compact things a bit; I know this wasn't what the initial code
> was doing, but it's an option.
>
> Something like:
>
> enum {
> GPIO_RWDR,
> GPIO_CONVERT,
> GPIO_BUSY,
> __GPIO_MAX,
> }
>
> Then, what you end up having is:
>
> struct ad7816_chip_info {
> struct spi_device *spi_dev;
> -u16 rdwr_pin;
> -u16 convert_pin;
> -u16 busy_pin;
> +struct gpio_desc *gpios[__GPIO_MAX];
>
>
> // Continued below
>
>
> > u8 oti_data[AD7816_CS_MAX + 1];
> > u8 channel_id; /* 0 always be temperature */
> > u8 mode;
> > @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> > *chip, u16 *data)
> > int ret = 0;
> > __be16 buf;
> >
> > - gpio_set_value(chip->rdwr_pin, 1);
> > - gpio_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
>
> Obviously, in the above context, this becomes:
> +gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
> +gpiod_set_value(chip->gpios[GPIO_RWDR], 0);
>
> > ret = spi_write(spi_dev, >channel_id, sizeof(chip-
> > > channel_id));
> >
> > if (ret < 0) {
> > dev_err(_dev->dev, "SPI channel setting error\n");
> > return ret;
> > }
> > - gpio_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> >
> > if (chip->mode == AD7816_PD) { /* operating mode 2 */
> > - gpio_set_value(chip->convert_pin, 1);
> > - gpio_set_value(chip->convert_pin, 0);
> > + gpiod_set_value(chip->convert_pin, 1);
> > + gpiod_set_value(chip->convert_pin, 0);
> > } else { /* operating mode 1 */
> > - gpio_set_value(chip->convert_pin, 0);
> > - gpio_set_value(chip->convert_pin, 1);
> > + gpiod_set_value(chip->convert_pin, 0);
> > + gpiod_set_value(chip->convert_pin, 1);
> > }
> >
> > - while (gpio_get_value(chip->busy_pin))
> > + while (gpiod_get_value(chip->busy_pin))
> > cpu_relax();
> >
> > - gpio_set_value(chip->rdwr_pin, 0);
> > - gpio_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > ret = spi_read(spi_dev, , sizeof(*data));
> > if (ret < 0) {
> > dev_err(_dev->dev, "SPI data read error\n");
> > @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> > *chip, u8 data)
> > struct spi_device *spi_dev = chip->spi_dev;
> > int ret = 0;
> >
> > - gpio_set_value(chip->rdwr_pin, 1);
> > - gpio_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 0);
> > ret = spi_write(spi_dev, , sizeof(data));
> > if (ret < 0)
> > dev_err(_dev->dev, "SPI oti data write error\n");
> > @@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device
> > *dev,
> > struct ad7816_chip_info *chip = iio_priv(indio_dev);
> >
> > if (strcmp(buf, "full")) {
> > - gpio_set_value(chip->rdwr_pin, 1);
> > + gpiod_set_value(chip->rdwr_pin, 1);
> > chip->mode = AD7816_FULL;
> > } else {
> > - gpio_set_value(chip->rdwr_pin, 0);
> > + gpiod_set_value(chip->rdwr_pin, 0);
> > chip->mode = AD7816_PD;
> > }
> >
> > @@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device
> > *spi_dev)
> > {
> > struct ad7816_chip_info *chip;
> > struct iio_dev *indio_dev;
> > - unsigned short *pins =
