Re: [PATCH] Staging: octeon-usb: octeon-hcd.c: Remove boiler plate and add SPDX
On Wed, Jun 13, 2018 at 05:28:25PM -0400, Javier Martinez wrote: > Removed massive boiler plate text at top of the file and instead > replaced it with a simple SPDX license identifier. > > Signed-off-by: Javier Martinez > --- > drivers/staging/octeon-usb/octeon-hcd.c | 47 ++--- > 1 file changed, 2 insertions(+), 45 deletions(-) > > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c > b/drivers/staging/octeon-usb/octeon-hcd.c > index cded30f145aa..df61e8321140 100644 > --- a/drivers/staging/octeon-usb/octeon-hcd.c > +++ b/drivers/staging/octeon-usb/octeon-hcd.c > @@ -1,48 +1,5 @@ > -/* > - * This file is subject to the terms and conditions of the GNU General Public > - * License. See the file "COPYING" in the main directory of this archive > - * for more details. > - * > - * Copyright (C) 2008 Cavium Networks > - * > - * Some parts of the code were originally released under BSD license: > - * > - * Copyright (c) 2003-2010 Cavium Networks (supp...@cavium.com). All rights > - * reserved. > - * > - * Redistribution and use in source and binary forms, with or without > - * modification, are permitted provided that the following conditions are > - * met: > - * > - * * Redistributions of source code must retain the above copyright > - * notice, this list of conditions and the following disclaimer. > - * > - * * Redistributions in binary form must reproduce the above > - * copyright notice, this list of conditions and the following > - * disclaimer in the documentation and/or other materials provided > - * with the distribution. > - * > - * * Neither the name of Cavium Networks nor the names of > - * its contributors may be used to endorse or promote products > - * derived from this software without specific prior written > - * permission. > - * > - * This Software, including technical data, may be subject to U.S. export > - * control laws, including the U.S. Export Administration Act and its > associated > - * regulations, and may be subject to export or import regulations in other > - * countries. > - * > - * TO THE MAXIMUM EXTENT PERMITTED BY LAW, THE SOFTWARE IS PROVIDED "AS IS" > - * AND WITH ALL FAULTS AND CAVIUM NETWORKS MAKES NO PROMISES, > REPRESENTATIONS OR > - * WARRANTIES, EITHER EXPRESS, IMPLIED, STATUTORY, OR OTHERWISE, WITH > RESPECT TO > - * THE SOFTWARE, INCLUDING ITS CONDITION, ITS CONFORMITY TO ANY > REPRESENTATION > - * OR DESCRIPTION, OR THE EXISTENCE OF ANY LATENT OR PATENT DEFECTS, AND > CAVIUM > - * SPECIFICALLY DISCLAIMS ALL IMPLIED (IF ANY) WARRANTIES OF TITLE, > - * MERCHANTABILITY, NONINFRINGEMENT, FITNESS FOR A PARTICULAR PURPOSE, LACK > OF > - * VIRUSES, ACCURACY OR COMPLETENESS, QUIET ENJOYMENT, QUIET POSSESSION OR > - * CORRESPONDENCE TO DESCRIPTION. THE ENTIRE RISK ARISING OUT OF USE OR > - * PERFORMANCE OF THE SOFTWARE LIES WITH YOU. > - */ > +// SPDX-License-Identifier: GNU GPL That is not a valid identifier, please read the documentation for how to do this properly. Also, and more importantly, you picked the wrong one. Do NOT mess with license texts if you don't know what you are doing... greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: pi433: fix race condition in pi433_ioctl
Hi Hugo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.17 next-20180613]
[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/Hugo-Lefeuvre/staging-pi433-fix-race-condition-in-pi433_ioctl/20180614-091011
config: i386-randconfig-s0-201823 (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=i386
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/string.h:3:0,
from include/linux/string.h:20,
from arch/x86/include/asm/page_32.h:35,
from arch/x86/include/asm/page.h:14,
from arch/x86/include/asm/thread_info.h:12,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:81,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:10,
from drivers/staging//pi433/pi433_if.c:31:
drivers/staging//pi433/pi433_if.c: In function 'pi433_ioctl':
>> drivers/staging//pi433/pi433_if.c:909:30: error: 'tx_cfg_buffer' undeclared
>> (first use in this function)
memcpy(>tx_cfg, _cfg_buffer, sizeof(struct pi433_tx_cfg));
^
arch/x86/include/asm/string_32.h:183:45: note: in definition of macro
'memcpy'
#define memcpy(t, f, n) __builtin_memcpy(t, f, n)
^
drivers/staging//pi433/pi433_if.c:909:30: note: each undeclared identifier
is reported only once for each function it appears in
memcpy(>tx_cfg, _cfg_buffer, sizeof(struct pi433_tx_cfg));
^
arch/x86/include/asm/string_32.h:183:45: note: in definition of macro
'memcpy'
#define memcpy(t, f, n) __builtin_memcpy(t, f, n)
^
vim +/tx_cfg_buffer +909 drivers/staging//pi433/pi433_if.c
876
877 static long
878 pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
879 {
880 int retval = 0;
881 struct pi433_instance *instance;
882 struct pi433_device *device;
883 struct pi433_tx_cfg tx_cfg;
884 void __user *argp = (void __user *)arg;
885
886 /* Check type and command number */
887 if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC)
888 return -ENOTTY;
889
890 /* TODO? guard against device removal before, or while,
891 * we issue this ioctl. --> device_get()
892 */
893 instance = filp->private_data;
894 device = instance->device;
895
896 if (!device)
897 return -ESHUTDOWN;
898
899 switch (cmd) {
900 case PI433_IOC_RD_TX_CFG:
901 if (copy_to_user(argp, >tx_cfg,
902 sizeof(struct pi433_tx_cfg)))
903 return -EFAULT;
904 break;
905 case PI433_IOC_WR_TX_CFG:
906 if (copy_from_user(_cfg, argp, sizeof(struct
pi433_tx_cfg)))
907 return -EFAULT;
908 mutex_lock(>tx_fifo_lock);
> 909 memcpy(>tx_cfg, _cfg_buffer, sizeof(struct
> pi433_tx_cfg));
910 mutex_unlock(>tx_fifo_lock);
911 break;
912 case PI433_IOC_RD_RX_CFG:
913 if (copy_to_user(argp, >rx_cfg,
914 sizeof(struct pi433_rx_cfg)))
915 return -EFAULT;
916 break;
917 case PI433_IOC_WR_RX_CFG:
918 mutex_lock(>rx_lock);
919
920 /* during pendig read request, change of config not
allowed */
921 if (device->rx_active) {
922 mutex_unlock(>rx_lock);
923 return -EAGAIN;
924 }
925
926 if (copy_from_user(>rx_cfg, argp,
927 sizeof(struct pi433_rx_cfg))) {
928 mutex_unlock(>rx_lock);
929 return -EFAULT;
930 }
931
932 mutex_unlock(>rx_lock);
933 break;
934 default:
935 retval = -
Re: [PATCH v3] staging: pi433: fix race condition in pi433_ioctl
Hi Hugo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v4.17 next-20180613]
[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/Hugo-Lefeuvre/staging-pi433-fix-race-condition-in-pi433_ioctl/20180614-091011
config: x86_64-randconfig-x018-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/staging/pi433/pi433_if.c: In function 'pi433_ioctl':
>> drivers/staging/pi433/pi433_if.c:909:30: error: 'tx_cfg_buffer' undeclared
>> (first use in this function); did you mean 'sg_copy_buffer'?
memcpy(>tx_cfg, _cfg_buffer, sizeof(struct pi433_tx_cfg));
^
sg_copy_buffer
drivers/staging/pi433/pi433_if.c:909:30: note: each undeclared identifier is
reported only once for each function it appears in
vim +909 drivers/staging/pi433/pi433_if.c
876
877 static long
878 pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
879 {
880 int retval = 0;
881 struct pi433_instance *instance;
882 struct pi433_device *device;
883 struct pi433_tx_cfg tx_cfg;
884 void __user *argp = (void __user *)arg;
885
886 /* Check type and command number */
887 if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC)
888 return -ENOTTY;
889
890 /* TODO? guard against device removal before, or while,
891 * we issue this ioctl. --> device_get()
892 */
893 instance = filp->private_data;
894 device = instance->device;
895
896 if (!device)
897 return -ESHUTDOWN;
898
899 switch (cmd) {
900 case PI433_IOC_RD_TX_CFG:
901 if (copy_to_user(argp, >tx_cfg,
902 sizeof(struct pi433_tx_cfg)))
903 return -EFAULT;
904 break;
905 case PI433_IOC_WR_TX_CFG:
906 if (copy_from_user(_cfg, argp, sizeof(struct
pi433_tx_cfg)))
907 return -EFAULT;
908 mutex_lock(>tx_fifo_lock);
> 909 memcpy(>tx_cfg, _cfg_buffer, sizeof(struct
> pi433_tx_cfg));
910 mutex_unlock(>tx_fifo_lock);
911 break;
912 case PI433_IOC_RD_RX_CFG:
913 if (copy_to_user(argp, >rx_cfg,
914 sizeof(struct pi433_rx_cfg)))
915 return -EFAULT;
916 break;
917 case PI433_IOC_WR_RX_CFG:
918 mutex_lock(>rx_lock);
919
920 /* during pendig read request, change of config not
allowed */
921 if (device->rx_active) {
922 mutex_unlock(>rx_lock);
923 return -EAGAIN;
924 }
925
926 if (copy_from_user(>rx_cfg, argp,
927 sizeof(struct pi433_rx_cfg))) {
928 mutex_unlock(>rx_lock);
929 return -EFAULT;
930 }
931
932 mutex_unlock(>rx_lock);
933 break;
934 default:
935 retval = -EINVAL;
936 }
937
938 return retval;
939 }
940
---
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 2/2] dt-bindings: document gpio-mt7621 bindings
On Wed, Jun 13, 2018 at 01:28:35PM -0600, Rob Herring wrote:
> On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos
> wrote:
> > Hi Rob,
> >
> > Thanks for your time in reviewing this.
> >
> > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring wrote:
> >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
> >>> Add a devicetree binding documentation for the mt7621 driver.
> >>
>
> >>> + second cell specifies GPIO flags, as defined in
> >>> .
> >>> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> >>> +- gpio-controller : Marks the device node as a GPIO controller.
> >>> +- reg : The id of the bank that the node describes.
> >>
> >> I'd prefer to not have banks defined in DT. Do you have a variable
> >> number or resources that are per bank? If not, then you don't need them.
> >
> > Mmmm, That's what I understood from documentation:
> >
> > "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
> > Usually each such bank is
> > exposed in the device tree as an individual gpio-controller node. ..."
>
> This should be conditioned on being able to divide up the registers by
> bank which seems like you can't. Or there's the case like the DW GPIO
> block and the number of banks is configurable.
I see. Thanks for clarifing this.
>
> > If this is not a good approach, could you please me point me out to a
> > device tree example where
> > the correct approach is being used?
>
> I'm not sure offhand. There are lots of examples of single nodes I'm
> sure. Which ones have banks I haven't a clue. IIRC, there were some
> cases where the bank # was part of the GPIO cells, but I seem to
> recall Linus prefers not having 3 cells.
Ok, so... does the following single node sounds acceptable?
gpio: gpio@600 {
#gpio-cells = <2>;
#interrupt-cells = <2>;
compatible = "mediatek,mt7621-gpio";
gpio-controller;
interrupt-controller;
reg = <0x600 0x60>;
interrupt-parent = <>;
interrupts = ;
mediatek,gpio-bank-widths = <32 32 32>;
}
Changing definition for "reg" and adding a new one for
"mediatek,gpio-bank-widths" as follows:
reg:
Define the base and range of the address space containing
the mediatek GPIO controller registers
mediatek,gpio-bank-widths:
Number of GPIO lines for each bank. Number of elements must
correspond to number of banks suggested by the 'reg' property.
Thanks in advance.
>
> Rob
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: pi433: fix race condition in pi433_ioctl
Hi Dan,
> There is no need for this comment, since it's obvious. Also if you use
> simpler names then the copy fits on one line:
>
> if (copy_from_user(_cfg, argp, sizeof(tx_cfg)) {
>
>
> > + mutex_lock(>tx_fifo_lock);
> > + if (copy_from_user(_cfg_buffer, argp,
> > + sizeof(struct pi433_tx_cfg))) {
>
> Sorry for the duplicate review, but it got sent to both my inboxes... :P
Thanks for your review ! Patch updated.
Please tell me if you don't to be CC-ed anymore. :)
regards,
Hugo
--
Hugo Lefeuvre (hle)|www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] staging: pi433: fix race condition in pi433_ioctl
In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is modified via copy_from_user(>tx_cfg, argp, sizeof(struct pi433_tx_cfg))) without any kind of synchronization. In the case where two threads would execute this same command concurrently the tx_cfg field might enter in an inconsistent state. Additionally: if ioctl(PI433_IOC_WR_TX_CFG) and write() execute concurrently the tx config might be modified while it is being copied to the fifo, resulting in potential data corruption. Fix: Get instance->tx_cfg_lock before modifying tx config in the PI433_IOC_WR_TX_CFG case in pi433_ioctl. Also, do not copy data directly from user space to instance->tx_cfg. Instead use a temporary buffer allowing future checks for correctness of copied data and simpler code. Signed-off-by: Hugo Lefeuvre --- Changes in v4: - Fix incorrect buffer name in memcpy. --- drivers/staging/pi433/pi433_if.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c index b061f77dda41..94e0bfcec991 100644 --- a/drivers/staging/pi433/pi433_if.c +++ b/drivers/staging/pi433/pi433_if.c @@ -880,6 +880,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) int retval = 0; struct pi433_instance *instance; struct pi433_device *device; + struct pi433_tx_cfg tx_cfg; void __user *argp = (void __user *)arg; /* Check type and command number */ @@ -902,9 +903,11 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return -EFAULT; break; case PI433_IOC_WR_TX_CFG: - if (copy_from_user(>tx_cfg, argp, - sizeof(struct pi433_tx_cfg))) + if (copy_from_user(_cfg, argp, sizeof(struct pi433_tx_cfg))) return -EFAULT; + mutex_lock(>tx_fifo_lock); + memcpy(>tx_cfg, _cfg, sizeof(struct pi433_tx_cfg)); + mutex_unlock(>tx_fifo_lock); break; case PI433_IOC_RD_RX_CFG: if (copy_to_user(argp, >rx_cfg, -- 2.17.1 signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] staging: pi433: fix race condition in pi433_ioctl
In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is
modified via
copy_from_user(>tx_cfg, argp, sizeof(struct pi433_tx_cfg)))
without any kind of synchronization. In the case where two threads
would execute this same command concurrently the tx_cfg field might
enter in an inconsistent state.
Additionally: if ioctl(PI433_IOC_WR_TX_CFG) and write() execute
concurrently the tx config might be modified while it is being
copied to the fifo, resulting in potential data corruption.
Fix: Get instance->tx_cfg_lock before modifying tx config in the
PI433_IOC_WR_TX_CFG case in pi433_ioctl.
Also, do not copy data directly from user space to instance->tx_cfg.
Instead use a temporary buffer allowing future checks for correctness
of copied data and simpler code.
Signed-off-by: Hugo Lefeuvre
---
Changes in v3:
- Use tx_cfg for the name of temporary buffer (shorter, allows
copy_from_user call to fit in a single line).
- Move mutex_{un,}lock calls around memcpy instead of before
copy_from_user call (was useless since we use a temporary buffer).
- Remove useless comment before mutex_lock.
---
drivers/staging/pi433/pi433_if.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b061f77dda41..53a69cb37056 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -880,6 +880,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned
long arg)
int retval = 0;
struct pi433_instance *instance;
struct pi433_device *device;
+ struct pi433_tx_cfg tx_cfg;
void __user *argp = (void __user *)arg;
/* Check type and command number */
@@ -902,9 +903,11 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned
long arg)
return -EFAULT;
break;
case PI433_IOC_WR_TX_CFG:
- if (copy_from_user(>tx_cfg, argp,
- sizeof(struct pi433_tx_cfg)))
+ if (copy_from_user(_cfg, argp, sizeof(struct pi433_tx_cfg)))
return -EFAULT;
+ mutex_lock(>tx_fifo_lock);
+ memcpy(>tx_cfg, _cfg_buffer, sizeof(struct
pi433_tx_cfg));
+ mutex_unlock(>tx_fifo_lock);
break;
case PI433_IOC_RD_RX_CFG:
if (copy_to_user(argp, >rx_cfg,
--
2.17.1
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()
> From: Bjorn Helgaas > Sent: Wednesday, June 13, 2018 15:15 > > ... > > It looks Lorenzo's pci.git tree has not been updated for 3+ weeks. > > I guess Lorenzo may be on vacation. > > > > @Bjorn, can this patch go through your tree? > > Should I resubmit it? > > No need to resubmit it, Lorenzo has been out for a bit, but I'm sure > he'll pick this up as he catches up. OK, I see. Thanks! > You might, however, fix the commit log: > > This is not an issue because hv_pci_onchannelcallback() is not slow, > and it not a hot path. > > This has at least one typo (I think you mean "and *is* not a hot > path"). Sorry -- yes, it's a typo. I hope Lorenzo can help to fix this, or I can resubmit it if Lorenzo or you want me to do it. > I also don't understand the sentence as a whole because the > hv_pci_onchannelcallback() comment says it's called whenever the host > sends a packet to this channel, and that *does* sound like a hot path. Sorry for not making it clear. The host only sends a packet into the channel of the guest when there is a change of device configuration (i.e. hot add or remove a device), or the host is responding to the guest's request. The change of device configuration is only triggered on-demand by the administrator on the host, and the guest's requests are one-off when the device is probed. So IMO the callback is not a hot path. > I also don't understand the "hv_pci_onchannelcallback() is not slow" > part. In other words, you're saying hv_pci_onchannelcallback() is > fast and it's not a hot path. And apparently this has something to do > with the difference between local_bh_disable() and local_irq_save()? > > Bjorn Actually in my original internal version of the patch, I did use local_irq_save/restore(). hv_pci_onchannelcallback() itself runs fast, but here since it's in a loop (i.e. the while (!try_wait_for_completion(_pkt.host_event) loop), IIRC I was asked if I really need local_irq_save/restore(), and I answered "not really", so later I switched to local_bh_disable()/enable(). However, recently I found that if we enable CONFIG_PROVE_LOCKING=y, the local_bh_enable() can trigger a warning because the function hv_compose_msi_msg() can be called with local IRQs disabled (BTW, hv_compose_msi_msg() can also be called with local IRQS enabled in another code path): IRQs not enabled as expected WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip Despite the warning, the code itself can still work correctly, but IMO we'd better switch back to local_irq_save/restore(), and hence I made the patch. I hope the explanation sounds reasonable. :-) Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
On Wed, 13 Jun 2018 22:03:34 + Yidong Ren wrote: > > From: devel On Behalf > > Of Stephen Hemminger > > > +/* statistics per queue (rx/tx packets/bytes) */ #define > > > +NETVSC_PCPU_STATS_LEN (num_present_cpus() * > > ARRAY_SIZE(pcpu_stats)) > > > > Even though Hyper-V/Azure does not support hot plug cpu's it might be > > better to num_cpu_possible to avoid any possible future surprises. > > That will create a very long output (num_cpu_possible = 128 on my machine) > for ethtool, > While doesn't provide additional info. > num_present_cpus() would cause problem only if someone removed cpu > between netvsc_get_sset_count() and netvsc_get_strings() and > netvsc_get_ethtool_stats(). > > An alternative way could be: Check all stats, and only output if not zero. > This need to be done in two pass. First pass to get the correct count, second > pass to print the number. > Is there an elegant way to do this? Ok, but there is a race between getting names and getting statistics. If a cpu was added/removed then statistics would not match. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()
On Wed, Jun 13, 2018 at 08:32:13PM +, Dexuan Cui wrote:
> > From: Dexuan Cui
> > Sent: Wednesday, June 6, 2018 17:15
> > To: Haiyang Zhang ; Lorenzo Pieralisi
> > ; Bjorn Helgaas ;
> > linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> > Hemminger ; o...@aepfle.de;
> > a...@canonical.com; jasow...@redhat.com
> > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > vkuzn...@redhat.com; marcelo.ce...@canonical.com
> > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> >
> > > From: Haiyang Zhang
> > > Sent: Friday, May 25, 2018 12:52
> > > To: Dexuan Cui ; Lorenzo Pieralisi
> > > ; Bjorn Helgaas ;
> > > linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> > > Hemminger ; o...@aepfle.de;
> > > a...@canonical.com; jasow...@redhat.com
> > > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > > vkuzn...@redhat.com; marcelo.ce...@canonical.com
> > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > hv_compose_msi_msg()
> > >
> > > > From: Dexuan Cui
> > > > Sent: Tuesday, May 22, 2018 8:18 PM
> > > > To: Lorenzo Pieralisi ; Bjorn Helgaas
> > > > ; linux-...@vger.kernel.org; KY Srinivasan
> > > > ; Stephen Hemminger ;
> > > > o...@aepfle.de; a...@canonical.com; jasow...@redhat.com
> > > > Cc: linux-ker...@vger.kernel.org;
> > > > driverdev-devel@linuxdriverproject.org;
> > > > Haiyang Zhang ; vkuzn...@redhat.com;
> > > > marcelo.ce...@canonical.com
> > > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > > hv_compose_msi_msg()
> > > >
> > > >
> > > > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > > hv_compose_msi_msg()")
> > > > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> > > also
> > > > run in tasklet context as the channel event callback.
> > > >
> > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert
> > > > IRQs
> > > are
> > > > disabled/enabled"), it turns out can we trigger a warning at the
> > > > beginning
> > of
> > > > __local_bh_enable_ip(), because the upper layer irq code can call
> > > > hv_compose_msi_msg() with local irqs disabled.
> > > >
> > > > Let's fix the warning by switching to local_irq_save()/restore(). This
> > > > is not an
> > > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot
> > > > path.
> > > >
> > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > hv_compose_msi_msg()")
> > > > Signed-off-by: Dexuan Cui
> > > > Cc:
> > > > Cc: Stephen Hemminger
> > > > Cc: K. Y. Srinivasan
> > > > ---
> > >
> > > Reviewed-by: Haiyang Zhang
> > >
> > > Thanks you.
> >
> > Hi Lorenzo,
> >
> > Can I have your reply to this patch?
> >
> > -- Dexuan
>
> It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> I guess Lorenzo may be on vacation.
>
> @Bjorn, can this patch go through your tree?
> Should I resubmit it?
No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
he'll pick this up as he catches up.
You might, however, fix the commit log:
This is not an issue because hv_pci_onchannelcallback() is not slow,
and it not a hot path.
This has at least one typo (I think you mean "and *is* not a hot
path").
I also don't understand the sentence as a whole because the
hv_pci_onchannelcallback() comment says it's called whenever the host
sends a packet to this channel, and that *does* sound like a hot path.
I also don't understand the "hv_pci_onchannelcallback() is not slow"
part. In other words, you're saying hv_pci_onchannelcallback() is
fast and it's not a hot path. And apparently this has something to do
with the difference between local_bh_disable() and local_irq_save()?
Bjorn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
> From: devel On Behalf > Of Stephen Hemminger > > +/* statistics per queue (rx/tx packets/bytes) */ #define > > +NETVSC_PCPU_STATS_LEN (num_present_cpus() * > ARRAY_SIZE(pcpu_stats)) > > Even though Hyper-V/Azure does not support hot plug cpu's it might be > better to num_cpu_possible to avoid any possible future surprises. That will create a very long output (num_cpu_possible = 128 on my machine) for ethtool, While doesn't provide additional info. num_present_cpus() would cause problem only if someone removed cpu between netvsc_get_sset_count() and netvsc_get_strings() and netvsc_get_ethtool_stats(). An alternative way could be: Check all stats, and only output if not zero. This need to be done in two pass. First pass to get the correct count, second pass to print the number. Is there an elegant way to do this? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
On Wed, 13 Jun 2018 12:36:08 -0700
Yidong Ren wrote:
> From: Yidong Ren
>
> This patch implements following ethtool stats fields for netvsc:
> cpu_tx/rx_packets/bytes
> cpu_vf_tx/rx_packets/bytes
>
> Corresponding per-cpu counters exist in current code. Exposing these
> counters will help troubleshooting performance issues.
>
> Signed-off-by: Yidong Ren
> ---
> Changes in v2:
> - Remove cpp style comment
> - Resubmit after freeze
>
> drivers/net/hyperv/hyperv_net.h | 11 +
> drivers/net/hyperv/netvsc_drv.c | 104
> +++-
> 2 files changed, 113 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 23304ac..c825353 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -873,6 +873,17 @@ struct netvsc_ethtool_stats {
> unsigned long wake_queue;
> };
>
> +struct netvsc_ethtool_pcpu_stats {
> + u64 rx_packets;
> + u64 rx_bytes;
> + u64 tx_packets;
> + u64 tx_bytes;
> + u64 vf_rx_packets;
> + u64 vf_rx_bytes;
> + u64 vf_tx_packets;
> + u64 vf_tx_bytes;
> +};
> +
> struct netvsc_vf_pcpu_stats {
> u64 rx_packets;
> u64 rx_bytes;
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 7b18a8c..6803aae 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1105,6 +1105,66 @@ static void netvsc_get_vf_stats(struct net_device *net,
> }
> }
>
> +static void netvsc_get_pcpu_stats(struct net_device *net,
> + struct netvsc_ethtool_pcpu_stats
> + __percpu *pcpu_tot)
> +{
> + struct net_device_context *ndev_ctx = netdev_priv(net);
> + struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev);
> + int i;
> +
> + /* fetch percpu stats of vf */
> + for_each_possible_cpu(i) {
> + const struct netvsc_vf_pcpu_stats *stats =
> + per_cpu_ptr(ndev_ctx->vf_stats, i);
> + struct netvsc_ethtool_pcpu_stats *this_tot =
> + per_cpu_ptr(pcpu_tot, i);
> + unsigned int start;
> +
> + do {
> + start = u64_stats_fetch_begin_irq(>syncp);
> + this_tot->vf_rx_packets = stats->rx_packets;
> + this_tot->vf_tx_packets = stats->tx_packets;
> + this_tot->vf_rx_bytes = stats->rx_bytes;
> + this_tot->vf_tx_bytes = stats->tx_bytes;
> + } while (u64_stats_fetch_retry_irq(>syncp, start));
> + this_tot->rx_packets = this_tot->vf_rx_packets;
> + this_tot->tx_packets = this_tot->vf_tx_packets;
> + this_tot->rx_bytes = this_tot->vf_rx_bytes;
> + this_tot->tx_bytes = this_tot->vf_tx_bytes;
> + }
> +
> + /* fetch percpu stats of netvsc */
> + for (i = 0; i < nvdev->num_chn; i++) {
> + const struct netvsc_channel *nvchan = >chan_table[i];
> + const struct netvsc_stats *stats;
> + struct netvsc_ethtool_pcpu_stats *this_tot =
> + per_cpu_ptr(pcpu_tot, nvchan->channel->target_cpu);
> + u64 packets, bytes;
> + unsigned int start;
> +
> + stats = >tx_stats;
> + do {
> + start = u64_stats_fetch_begin_irq(>syncp);
> + packets = stats->packets;
> + bytes = stats->bytes;
> + } while (u64_stats_fetch_retry_irq(>syncp, start));
> +
> + this_tot->tx_bytes += bytes;
> + this_tot->tx_packets+= packets;
> +
> + stats = >rx_stats;
> + do {
> + start = u64_stats_fetch_begin_irq(>syncp);
> + packets = stats->packets;
> + bytes = stats->bytes;
> + } while (u64_stats_fetch_retry_irq(>syncp, start));
> +
> + this_tot->rx_bytes += bytes;
> + this_tot->rx_packets+= packets;
> + }
> +}
> +
> static void netvsc_get_stats64(struct net_device *net,
> struct rtnl_link_stats64 *t)
> {
> @@ -1202,6 +1262,23 @@ static const struct {
> { "rx_no_memory", offsetof(struct netvsc_ethtool_stats, rx_no_memory) },
> { "stop_queue", offsetof(struct netvsc_ethtool_stats, stop_queue) },
> { "wake_queue", offsetof(struct netvsc_ethtool_stats, wake_queue) },
> +}, pcpu_stats[] = {
> + { "cpu%u_rx_packets",
> + offsetof(struct netvsc_ethtool_pcpu_stats, rx_packets) },
> + { "cpu%u_rx_bytes",
> + offsetof(struct netvsc_ethtool_pcpu_stats, rx_bytes) },
> + { "cpu%u_tx_packets",
> + offsetof(struct netvsc_ethtool_pcpu_stats, tx_packets) },
> + { "cpu%u_tx_bytes",
> + offsetof(struct
[PATCH] Staging: octeon-usb: octeon-hcd.c: Remove boiler plate and add SPDX
Removed massive boiler plate text at top of the file and instead replaced it with a simple SPDX license identifier. Signed-off-by: Javier Martinez --- drivers/staging/octeon-usb/octeon-hcd.c | 47 ++--- 1 file changed, 2 insertions(+), 45 deletions(-) diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c index cded30f145aa..df61e8321140 100644 --- a/drivers/staging/octeon-usb/octeon-hcd.c +++ b/drivers/staging/octeon-usb/octeon-hcd.c @@ -1,48 +1,5 @@ -/* - * This file is subject to the terms and conditions of the GNU General Public - * License. See the file "COPYING" in the main directory of this archive - * for more details. - * - * Copyright (C) 2008 Cavium Networks - * - * Some parts of the code were originally released under BSD license: - * - * Copyright (c) 2003-2010 Cavium Networks (supp...@cavium.com). All rights - * reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials provided - * with the distribution. - * - * * Neither the name of Cavium Networks nor the names of - * its contributors may be used to endorse or promote products - * derived from this software without specific prior written - * permission. - * - * This Software, including technical data, may be subject to U.S. export - * control laws, including the U.S. Export Administration Act and its associated - * regulations, and may be subject to export or import regulations in other - * countries. - * - * TO THE MAXIMUM EXTENT PERMITTED BY LAW, THE SOFTWARE IS PROVIDED "AS IS" - * AND WITH ALL FAULTS AND CAVIUM NETWORKS MAKES NO PROMISES, REPRESENTATIONS OR - * WARRANTIES, EITHER EXPRESS, IMPLIED, STATUTORY, OR OTHERWISE, WITH RESPECT TO - * THE SOFTWARE, INCLUDING ITS CONDITION, ITS CONFORMITY TO ANY REPRESENTATION - * OR DESCRIPTION, OR THE EXISTENCE OF ANY LATENT OR PATENT DEFECTS, AND CAVIUM - * SPECIFICALLY DISCLAIMS ALL IMPLIED (IF ANY) WARRANTIES OF TITLE, - * MERCHANTABILITY, NONINFRINGEMENT, FITNESS FOR A PARTICULAR PURPOSE, LACK OF - * VIRUSES, ACCURACY OR COMPLETENESS, QUIET ENJOYMENT, QUIET POSSESSION OR - * CORRESPONDENCE TO DESCRIPTION. THE ENTIRE RISK ARISING OUT OF USE OR - * PERFORMANCE OF THE SOFTWARE LIES WITH YOU. - */ +// SPDX-License-Identifier: GNU GPL + #include #include -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
> From: Eric Dumazet > You actually want to allocate memory local to this cpu, possibly in one chunk, > not spread all over the places. > > kvmalloc(nr_cpu_ids * sizeof(struct netvsc_ethtool_pcpu_stats)) should be > really better, since it would most of the time be satisfied by a single > kmalloc() Got it. I'm just trying to allocate memory for each cpu. It doesn't have to be __percpu variable. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
On 06/13/2018 12:36 PM, Yidong Ren wrote:
> From: Yidong Ren
>
> This patch implements following ethtool stats fields for netvsc:
> cpu_tx/rx_packets/bytes
> cpu_vf_tx/rx_packets/bytes
...
>
> + pcpu_sum = alloc_percpu(struct netvsc_ethtool_pcpu_stats);
> + netvsc_get_pcpu_stats(dev, pcpu_sum);
> + for_each_present_cpu(cpu) {
> + struct netvsc_ethtool_pcpu_stats *this_sum =
> + per_cpu_ptr(pcpu_sum, cpu);
> + for (j = 0; j < ARRAY_SIZE(pcpu_stats); j++)
> + data[i++] = *(u64 *)((void *)this_sum
> + + pcpu_stats[j].offset);
> + }
> + free_percpu(pcpu_sum);
>
Using alloc_percpu() / free_percpu() for a short section of code makes no sense.
You actually want to allocate memory local to this cpu, possibly in one chunk,
not spread all over the places.
kvmalloc(nr_cpu_ids * sizeof(struct netvsc_ethtool_pcpu_stats)) should be
really better,
since it would most of the time be satisfied by a single kmalloc()
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon-usb: octeon-hcd.c: style fix line length warning
On Wed, Jun 13, 2018 at 04:40:15PM -0400, Javier Martinez wrote: > Checkpatch.pl issued a warning in the top of the commment within > octeon-hcd.c. This is a simple style fix for that. > > Signed-off-by: Javier Martinez > --- > drivers/staging/octeon-usb/octeon-hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c > b/drivers/staging/octeon-usb/octeon-hcd.c > index cded30f145aa..768b0148f3d5 100644 > --- a/drivers/staging/octeon-usb/octeon-hcd.c > +++ b/drivers/staging/octeon-usb/octeon-hcd.c > @@ -1,5 +1,5 @@ > /* > - * This file is subject to the terms and conditions of the GNU General Public > + * This file is subject to the terms and conditions of the GPL > * License. See the file "COPYING" in the main directory of this archive The grammer is now incorrect :( Shouldn't we just have a SPDX line at the top here and this whole boiler plate text be removed? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: octeon-usb: octeon-hcd.c: style fix line length warning
Checkpatch.pl issued a warning in the top of the commment within octeon-hcd.c. This is a simple style fix for that. Signed-off-by: Javier Martinez --- drivers/staging/octeon-usb/octeon-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c index cded30f145aa..768b0148f3d5 100644 --- a/drivers/staging/octeon-usb/octeon-hcd.c +++ b/drivers/staging/octeon-usb/octeon-hcd.c @@ -1,5 +1,5 @@ /* - * This file is subject to the terms and conditions of the GNU General Public + * This file is subject to the terms and conditions of the GPL * License. See the file "COPYING" in the main directory of this archive * for more details. * -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()
> From: Dexuan Cui
> Sent: Wednesday, June 6, 2018 17:15
> To: Haiyang Zhang ; Lorenzo Pieralisi
> ; Bjorn Helgaas ;
> linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> Hemminger ; o...@aepfle.de;
> a...@canonical.com; jasow...@redhat.com
> Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> vkuzn...@redhat.com; marcelo.ce...@canonical.com
> Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> hv_compose_msi_msg()
>
> > From: Haiyang Zhang
> > Sent: Friday, May 25, 2018 12:52
> > To: Dexuan Cui ; Lorenzo Pieralisi
> > ; Bjorn Helgaas ;
> > linux-...@vger.kernel.org; KY Srinivasan ; Stephen
> > Hemminger ; o...@aepfle.de;
> > a...@canonical.com; jasow...@redhat.com
> > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > vkuzn...@redhat.com; marcelo.ce...@canonical.com
> > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > hv_compose_msi_msg()
> >
> > > From: Dexuan Cui
> > > Sent: Tuesday, May 22, 2018 8:18 PM
> > > To: Lorenzo Pieralisi ; Bjorn Helgaas
> > > ; linux-...@vger.kernel.org; KY Srinivasan
> > > ; Stephen Hemminger ;
> > > o...@aepfle.de; a...@canonical.com; jasow...@redhat.com
> > > Cc: linux-ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> > > Haiyang Zhang ; vkuzn...@redhat.com;
> > > marcelo.ce...@canonical.com
> > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in
> > > hv_compose_msi_msg()
> > >
> > >
> > > Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> > hv_compose_msi_msg()")
> > > uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> > also
> > > run in tasklet context as the channel event callback.
> > >
> > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that
> > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs
> > are
> > > disabled/enabled"), it turns out can we trigger a warning at the beginning
> of
> > > __local_bh_enable_ip(), because the upper layer irq code can call
> > > hv_compose_msi_msg() with local irqs disabled.
> > >
> > > Let's fix the warning by switching to local_irq_save()/restore(). This is
> > > not an
> > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot
> > > path.
> > >
> > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in
> hv_compose_msi_msg()")
> > > Signed-off-by: Dexuan Cui
> > > Cc:
> > > Cc: Stephen Hemminger
> > > Cc: K. Y. Srinivasan
> > > ---
> >
> > Reviewed-by: Haiyang Zhang
> >
> > Thanks you.
>
> Hi Lorenzo,
>
> Can I have your reply to this patch?
>
> -- Dexuan
It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
I guess Lorenzo may be on vacation.
@Bjorn, can this patch go through your tree?
Should I resubmit it?
Thanks,
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc
From: Yidong Ren
This patch implements following ethtool stats fields for netvsc:
cpu_tx/rx_packets/bytes
cpu_vf_tx/rx_packets/bytes
Corresponding per-cpu counters exist in current code. Exposing these
counters will help troubleshooting performance issues.
Signed-off-by: Yidong Ren
---
Changes in v2:
- Remove cpp style comment
- Resubmit after freeze
drivers/net/hyperv/hyperv_net.h | 11 +
drivers/net/hyperv/netvsc_drv.c | 104 +++-
2 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 23304ac..c825353 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -873,6 +873,17 @@ struct netvsc_ethtool_stats {
unsigned long wake_queue;
};
+struct netvsc_ethtool_pcpu_stats {
+ u64 rx_packets;
+ u64 rx_bytes;
+ u64 tx_packets;
+ u64 tx_bytes;
+ u64 vf_rx_packets;
+ u64 vf_rx_bytes;
+ u64 vf_tx_packets;
+ u64 vf_tx_bytes;
+};
+
struct netvsc_vf_pcpu_stats {
u64 rx_packets;
u64 rx_bytes;
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 7b18a8c..6803aae 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1105,6 +1105,66 @@ static void netvsc_get_vf_stats(struct net_device *net,
}
}
+static void netvsc_get_pcpu_stats(struct net_device *net,
+ struct netvsc_ethtool_pcpu_stats
+ __percpu *pcpu_tot)
+{
+ struct net_device_context *ndev_ctx = netdev_priv(net);
+ struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev);
+ int i;
+
+ /* fetch percpu stats of vf */
+ for_each_possible_cpu(i) {
+ const struct netvsc_vf_pcpu_stats *stats =
+ per_cpu_ptr(ndev_ctx->vf_stats, i);
+ struct netvsc_ethtool_pcpu_stats *this_tot =
+ per_cpu_ptr(pcpu_tot, i);
+ unsigned int start;
+
+ do {
+ start = u64_stats_fetch_begin_irq(>syncp);
+ this_tot->vf_rx_packets = stats->rx_packets;
+ this_tot->vf_tx_packets = stats->tx_packets;
+ this_tot->vf_rx_bytes = stats->rx_bytes;
+ this_tot->vf_tx_bytes = stats->tx_bytes;
+ } while (u64_stats_fetch_retry_irq(>syncp, start));
+ this_tot->rx_packets = this_tot->vf_rx_packets;
+ this_tot->tx_packets = this_tot->vf_tx_packets;
+ this_tot->rx_bytes = this_tot->vf_rx_bytes;
+ this_tot->tx_bytes = this_tot->vf_tx_bytes;
+ }
+
+ /* fetch percpu stats of netvsc */
+ for (i = 0; i < nvdev->num_chn; i++) {
+ const struct netvsc_channel *nvchan = >chan_table[i];
+ const struct netvsc_stats *stats;
+ struct netvsc_ethtool_pcpu_stats *this_tot =
+ per_cpu_ptr(pcpu_tot, nvchan->channel->target_cpu);
+ u64 packets, bytes;
+ unsigned int start;
+
+ stats = >tx_stats;
+ do {
+ start = u64_stats_fetch_begin_irq(>syncp);
+ packets = stats->packets;
+ bytes = stats->bytes;
+ } while (u64_stats_fetch_retry_irq(>syncp, start));
+
+ this_tot->tx_bytes += bytes;
+ this_tot->tx_packets+= packets;
+
+ stats = >rx_stats;
+ do {
+ start = u64_stats_fetch_begin_irq(>syncp);
+ packets = stats->packets;
+ bytes = stats->bytes;
+ } while (u64_stats_fetch_retry_irq(>syncp, start));
+
+ this_tot->rx_bytes += bytes;
+ this_tot->rx_packets+= packets;
+ }
+}
+
static void netvsc_get_stats64(struct net_device *net,
struct rtnl_link_stats64 *t)
{
@@ -1202,6 +1262,23 @@ static const struct {
{ "rx_no_memory", offsetof(struct netvsc_ethtool_stats, rx_no_memory) },
{ "stop_queue", offsetof(struct netvsc_ethtool_stats, stop_queue) },
{ "wake_queue", offsetof(struct netvsc_ethtool_stats, wake_queue) },
+}, pcpu_stats[] = {
+ { "cpu%u_rx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, rx_packets) },
+ { "cpu%u_rx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, rx_bytes) },
+ { "cpu%u_tx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, tx_packets) },
+ { "cpu%u_tx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, tx_bytes) },
+ { "cpu%u_vf_rx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_packets) },
+ {
Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings
On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos wrote: > Hi Rob, > > Thanks for your time in reviewing this. > > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring wrote: >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote: >>> Add a devicetree binding documentation for the mt7621 driver. >> >>> + second cell specifies GPIO flags, as defined in >>> . >>> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported. >>> +- gpio-controller : Marks the device node as a GPIO controller. >>> +- reg : The id of the bank that the node describes. >> >> I'd prefer to not have banks defined in DT. Do you have a variable >> number or resources that are per bank? If not, then you don't need them. > > Mmmm, That's what I understood from documentation: > > "Some system-on-chips (SoCs) use the concept of GPIO banks. ... > Usually each such bank is > exposed in the device tree as an individual gpio-controller node. ..." This should be conditioned on being able to divide up the registers by bank which seems like you can't. Or there's the case like the DW GPIO block and the number of banks is configurable. > If this is not a good approach, could you please me point me out to a > device tree example where > the correct approach is being used? I'm not sure offhand. There are lots of examples of single nodes I'm sure. Which ones have banks I haven't a clue. IIRC, there were some cases where the bank # was part of the GPIO cells, but I seem to recall Linus prefers not having 3 cells. Rob ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
On Wed, Jun 13, 2018 at 08:26:43PM +0200, Chris Opperman wrote: > Hi Dan/Ian, > > Noted your comments regarding additional text, thanks! Just curious whether > the "scissors" format given at the link below is valid? > > https://kernelnewbies.org/PatchTipsAndTricks > > It is given as an alternative to placing additional text below the > cut-off line. > Try it yourself. Save your email as email.txt and then `cat email.txt | git am` and then review the patch with git log -p. I've seen people do the scissors thing, but I assume the maintainer has to hand edit the log which we refuse to do. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: comedi: ssv_dnp: fixed style line length warning
Fixed style line length warning detected by checkpatch.pl in the file ssv_dnp.c. Signed-off-by: Javier Martinez --- drivers/staging/comedi/drivers/ssv_dnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c index 0628060e42ca..87f46e0eb9ee 100644 --- a/drivers/staging/comedi/drivers/ssv_dnp.c +++ b/drivers/staging/comedi/drivers/ssv_dnp.c @@ -16,7 +16,7 @@ * Status: unknown */ -/* include files --- */ +/* include files -- */ #include #include "../comedidev.h" -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: comedi: ssv_dnp: fixed style line length warning
Fixed style line length warning detected by checkpatch.pl in the file ssv_dnp.c. Signed-off-by: Javier Martinez --- drivers/staging/comedi/drivers/ssv_dnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c index 0628060e42ca..87f46e0eb9ee 100644 --- a/drivers/staging/comedi/drivers/ssv_dnp.c +++ b/drivers/staging/comedi/drivers/ssv_dnp.c @@ -16,7 +16,7 @@ * Status: unknown */ -/* include files --- */ +/* include files -- */ #include #include "../comedidev.h" -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5] staging: comedi: Improved readability of function comedi_nsamples_left.
On 13/06/18 18:14, Chris Opperman wrote:
Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.
Signed-off-by: Chris Opperman
---
Changes v5:
a) Moved additional text to below the cut-off line.
The "Changes v5" text is a bit incomplete without the earlier change
history, but let's forget that since the previous change history was a
bit messed up anyway.
drivers/staging/comedi/drivers.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice
*s,
{
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+ unsigned long long scans_left;
+ unsigned long long samples_left;
- if (cmd->stop_src == TRIG_COUNT) {
- unsigned int scans_left = __comedi_nscans_left(s,
cmd->stop_arg);
- unsigned int scan_pos =
- comedi_bytes_to_samples(s, async->scan_progress);
- unsigned long long samples_left = 0;
-
- if (scans_left) {
- samples_left = ((unsigned long long)scans_left *
- cmd->scan_end_arg) - scan_pos;
- }
+ if (cmd->stop_src != TRIG_COUNT)
+ return nsamples;
- if (samples_left < nsamples)
- nsamples = samples_left;
- }
+ scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+ if (!scans_left)
+ return 0;
+
+ samples_left = scans_left * cmd->scan_end_arg -
+ comedi_bytes_to_samples(s, async->scan_progress);
+
+ if (samples_left < nsamples)
+ return samples_left;
return nsamples;
}
EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4
The actual patch looks fine thanks!
Reviewed-by: Ian Abbott
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] staging: rts5208: add error handling into rtsx_probe
If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.
Patch adds error handling into rtsx_probe.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev
---
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.
I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
drivers/staging/rts5208/rtsx.c | 38 +++---
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
if (!dev->chip) {
err = -ENOMEM;
- goto errout;
+ goto chip_alloc_fail;
}
spin_lock_init(>reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->remap_addr) {
dev_err(>dev, "ioremap error\n");
err = -ENXIO;
- goto errout;
+ goto ioremap_fail;
}
/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->rtsx_resv_buf) {
dev_err(>dev, "alloc dma buffer fail\n");
err = -ENXIO;
- goto errout;
+ goto dma_alloc_fail;
}
dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (rtsx_acquire_irq(dev) < 0) {
err = -EBUSY;
- goto errout;
+ goto irq_acquire_fail;
}
pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start control thread\n");
err = PTR_ERR(th);
- goto errout;
+ goto control_thread_fail;
}
dev->ctl_thread = th;
err = scsi_add_host(host, >dev);
if (err) {
dev_err(>dev, "Unable to add the scsi host\n");
- goto errout;
+ goto scsi_add_host_fail;
}
/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-scanning
thread\n");
complete(>scanning_done);
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
/* Start up the thread for polling thread */
th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-polling
thread\n");
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
dev->polling_thread = th;
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
return 0;
/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+ quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+ complete(>cmnd_ready);
+ wait_for_completion(>control_exit);
+control_thread_fail:
+ free_irq(dev->irq, (void *)dev);
+ rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+ dev->chip->host_cmds_ptr = NULL;
+ dev->chip->host_sg_tbl_ptr = NULL;
+ if (dev->chip->msi_en)
+ pci_disable_msi(dev->pci);
+dma_alloc_fail:
+ iounmap(dev->remap_addr);
+ioremap_fail:
+ kfree(dev->chip);
+chip_alloc_fail:
dev_err(>dev, "%s failed\n", __func__);
- release_everything(dev);
return err;
}
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
On 13/06/18 19:26, Chris Opperman wrote: Hi Dan/Ian, Noted your comments regarding additional text, thanks! Just curious whether the "scissors" format given at the link below is valid? https://kernelnewbies.org/PatchTipsAndTricks It is given as an alternative to placing additional text below the cut-off line. I don't know. Maybe? I haven't seen it used before. -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[no subject]
Subject: [PATCH v3] staging: rts5208: add error handling into rtsx_probe
If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.
Patch adds error handling into rtsx_probe.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev
---
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.
I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
drivers/staging/rts5208/rtsx.c | 38 +++---
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
if (!dev->chip) {
err = -ENOMEM;
- goto errout;
+ goto chip_alloc_fail;
}
spin_lock_init(>reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->remap_addr) {
dev_err(>dev, "ioremap error\n");
err = -ENXIO;
- goto errout;
+ goto ioremap_fail;
}
/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->rtsx_resv_buf) {
dev_err(>dev, "alloc dma buffer fail\n");
err = -ENXIO;
- goto errout;
+ goto dma_alloc_fail;
}
dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (rtsx_acquire_irq(dev) < 0) {
err = -EBUSY;
- goto errout;
+ goto irq_acquire_fail;
}
pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start control thread\n");
err = PTR_ERR(th);
- goto errout;
+ goto control_thread_fail;
}
dev->ctl_thread = th;
err = scsi_add_host(host, >dev);
if (err) {
dev_err(>dev, "Unable to add the scsi host\n");
- goto errout;
+ goto scsi_add_host_fail;
}
/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-scanning
thread\n");
complete(>scanning_done);
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
/* Start up the thread for polling thread */
th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-polling
thread\n");
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
dev->polling_thread = th;
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
return 0;
/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+ quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+ complete(>cmnd_ready);
+ wait_for_completion(>control_exit);
+control_thread_fail:
+ free_irq(dev->irq, (void *)dev);
+ rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+ dev->chip->host_cmds_ptr = NULL;
+ dev->chip->host_sg_tbl_ptr = NULL;
+ if (dev->chip->msi_en)
+ pci_disable_msi(dev->pci);
+dma_alloc_fail:
+ iounmap(dev->remap_addr);
+ioremap_fail:
+ kfree(dev->chip);
+chip_alloc_fail:
dev_err(>dev, "%s failed\n", __func__);
- release_everything(dev);
return err;
}
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v5] staging: comedi: Improved readability of function comedi_nsamples_left.
Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.
Signed-off-by: Chris Opperman
---
Changes v5:
a) Moved additional text to below the cut-off line.
drivers/staging/comedi/drivers.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice
*s,
{
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+ unsigned long long scans_left;
+ unsigned long long samples_left;
- if (cmd->stop_src == TRIG_COUNT) {
- unsigned int scans_left = __comedi_nscans_left(s,
cmd->stop_arg);
- unsigned int scan_pos =
- comedi_bytes_to_samples(s, async->scan_progress);
- unsigned long long samples_left = 0;
-
- if (scans_left) {
- samples_left = ((unsigned long long)scans_left *
- cmd->scan_end_arg) - scan_pos;
- }
+ if (cmd->stop_src != TRIG_COUNT)
+ return nsamples;
- if (samples_left < nsamples)
- nsamples = samples_left;
- }
+ scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+ if (!scans_left)
+ return 0;
+
+ samples_left = scans_left * cmd->scan_end_arg -
+ comedi_bytes_to_samples(s, async->scan_progress);
+
+ if (samples_left < nsamples)
+ return samples_left;
return nsamples;
}
EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rts5208: add check on NULL before dereference
On Wed, Jun 13, 2018 at 7:55 PM, Anton Vasilyev wrote:
> If rtsx_probe() fails to allocate dev->chip, then NULL pointer
> dereference occurs at release_everything()->rtsx_release_resources().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
You forgot to adjust subject and commit message to the new reality
which is brought by this change.
> Signed-off-by: Anton Vasilyev
> ---
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
>
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
> drivers/staging/rts5208/rtsx.c | 38 +++---
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..69e6abe14abf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
> dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
> if (!dev->chip) {
> err = -ENOMEM;
> - goto errout;
> + goto chip_alloc_fail;
> }
>
> spin_lock_init(>reg_lock);
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
> if (!dev->remap_addr) {
> dev_err(>dev, "ioremap error\n");
> err = -ENXIO;
> - goto errout;
> + goto ioremap_fail;
> }
>
> /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
> if (!dev->rtsx_resv_buf) {
> dev_err(>dev, "alloc dma buffer fail\n");
> err = -ENXIO;
> - goto errout;
> + goto dma_alloc_fail;
> }
> dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
> dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
> if (rtsx_acquire_irq(dev) < 0) {
> err = -EBUSY;
> - goto errout;
> + goto irq_acquire_fail;
> }
>
> pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
> if (IS_ERR(th)) {
> dev_err(>dev, "Unable to start control thread\n");
> err = PTR_ERR(th);
> - goto errout;
> + goto control_thread_fail;
> }
> dev->ctl_thread = th;
>
> err = scsi_add_host(host, >dev);
> if (err) {
> dev_err(>dev, "Unable to add the scsi host\n");
> - goto errout;
> + goto scsi_add_host_fail;
> }
>
> /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
> if (IS_ERR(th)) {
> dev_err(>dev, "Unable to start the device-scanning
> thread\n");
> complete(>scanning_done);
> - quiesce_and_remove_host(dev);
> err = PTR_ERR(th);
> - goto errout;
> + goto scan_thread_fail;
> }
>
> /* Start up the thread for polling thread */
> th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
> if (IS_ERR(th)) {
> dev_err(>dev, "Unable to start the device-polling
> thread\n");
> - quiesce_and_remove_host(dev);
> err = PTR_ERR(th);
> - goto errout;
> + goto scan_thread_fail;
> }
> dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
> return 0;
>
> /* We come here if there are any problems */
> -errout:
> +scan_thread_fail:
> + quiesce_and_remove_host(dev);
> +scsi_add_host_fail:
> + complete(>cmnd_ready);
> + wait_for_completion(>control_exit);
> +control_thread_fail:
> + free_irq(dev->irq, (void *)dev);
> + rtsx_release_chip(dev->chip);
> +irq_acquire_fail:
> + dev->chip->host_cmds_ptr = NULL;
> + dev->chip->host_sg_tbl_ptr = NULL;
> + if (dev->chip->msi_en)
> + pci_disable_msi(dev->pci);
> +dma_alloc_fail:
> + iounmap(dev->remap_addr);
> +ioremap_fail:
> + kfree(dev->chip);
> +chip_alloc_fail:
> dev_err(>dev, "%s failed\n", __func__);
> - release_everything(dev);
>
> return err;
> }
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
[PATCH v2] staging: rts5208: add check on NULL before dereference
If rtsx_probe() fails to allocate dev->chip, then NULL pointer
dereference occurs at release_everything()->rtsx_release_resources().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev
---
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.
I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
drivers/staging/rts5208/rtsx.c | 38 +++---
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
if (!dev->chip) {
err = -ENOMEM;
- goto errout;
+ goto chip_alloc_fail;
}
spin_lock_init(>reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->remap_addr) {
dev_err(>dev, "ioremap error\n");
err = -ENXIO;
- goto errout;
+ goto ioremap_fail;
}
/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (!dev->rtsx_resv_buf) {
dev_err(>dev, "alloc dma buffer fail\n");
err = -ENXIO;
- goto errout;
+ goto dma_alloc_fail;
}
dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
if (rtsx_acquire_irq(dev) < 0) {
err = -EBUSY;
- goto errout;
+ goto irq_acquire_fail;
}
pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start control thread\n");
err = PTR_ERR(th);
- goto errout;
+ goto control_thread_fail;
}
dev->ctl_thread = th;
err = scsi_add_host(host, >dev);
if (err) {
dev_err(>dev, "Unable to add the scsi host\n");
- goto errout;
+ goto scsi_add_host_fail;
}
/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-scanning
thread\n");
complete(>scanning_done);
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
/* Start up the thread for polling thread */
th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
if (IS_ERR(th)) {
dev_err(>dev, "Unable to start the device-polling
thread\n");
- quiesce_and_remove_host(dev);
err = PTR_ERR(th);
- goto errout;
+ goto scan_thread_fail;
}
dev->polling_thread = th;
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
return 0;
/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+ quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+ complete(>cmnd_ready);
+ wait_for_completion(>control_exit);
+control_thread_fail:
+ free_irq(dev->irq, (void *)dev);
+ rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+ dev->chip->host_cmds_ptr = NULL;
+ dev->chip->host_sg_tbl_ptr = NULL;
+ if (dev->chip->msi_en)
+ pci_disable_msi(dev->pci);
+dma_alloc_fail:
+ iounmap(dev->remap_addr);
+ioremap_fail:
+ kfree(dev->chip);
+chip_alloc_fail:
dev_err(>dev, "%s failed\n", __func__);
- release_everything(dev);
return err;
}
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
Hi Dan/Ian, Noted your comments regarding additional text, thanks! Just curious whether the "scissors" format given at the link below is valid? https://kernelnewbies.org/PatchTipsAndTricks It is given as an alternative to placing additional text below the cut-off line. Kind Regards, Chris Opperman ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings
Hi Rob,
Thanks for your time in reviewing this.
On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring wrote:
> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
>> Add a devicetree binding documentation for the mt7621 driver.
>
> Bindings are for h/w, not a driver.
You are totally right, my english is not my best as you can see :-).
I'll fix this
for v2.
>
>> Signed-off-by: Sergio Paracuellos
>> Reviewed-by: NeilBrown
>
> Space ^
>
Actually, this is deliberate, so it will not change.
>> ---
>> .../bindings/gpio/mediatek,mt7621-gpio.txt | 68
>> ++
>> 1 file changed, 68 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> new file mode 100644
>> index 000..30d8a02
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> @@ -0,0 +1,68 @@
>> +Mediatek SoC GPIO controller bindings
>> +
>> +The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
>> +The registers of all the banks are interwoven inside one single IO range.
>> +We load one GPIO controller instance per bank. To make this possible
>> +we support 2 types of nodes. The parent node defines the memory I/O range
>> and
>> +has 3 children each describing a single bank. Also the GPIO controller can
>> receive
>> +interrupts on any of the GPIOs, either edge or level. It then interrupts
>> the CPU
>> +using GIC INT12.
>> +
>> +Required properties for the top level node:
>> +- compatible:
>> + - "mediatek,mt7621-gpio" for Mediatek controllers
>> +- reg : Physical base address and length of the controller's registers
>> +- interrupt-parent : phandle of the parent interrupt controller.
>> +- interrupts : Interrupt specifier for the controllers interrupt.
>> +- interrupt-controller : Mark the device node as an interrupt controller.
>> +- #interrupt-cells : Should be 2. The first cell defines the interrupt
>> number.
>> + The second cell bits[3:0] is used to specify trigger type as follows:
>> + - 1 = low-to-high edge triggered.
>> + - 2 = high-to-low edge triggered.
>> + - 4 = active high level-sensitive.
>> + - 8 = active low level-sensitive.
>
> Just refer to the common definition.
ok, thanks. I will.
>
>> +
>> +
>> +Required properties for the GPIO bank node:
>> +- compatible:
>> + - "mediatek,mt7621-gpio-bank" for Mediatek banks
>> +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the
>
> So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31
>
> That doesn't seem ideal.
Yes, that's true. Actually this is one of the things that has been
changed for v2.
>
>> + second cell specifies GPIO flags, as defined in
>> .
>> + Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- reg : The id of the bank that the node describes.
>
> I'd prefer to not have banks defined in DT. Do you have a variable
> number or resources that are per bank? If not, then you don't need them.
Mmmm, That's what I understood from documentation:
"Some system-on-chips (SoCs) use the concept of GPIO banks. ...
Usually each such bank is
exposed in the device tree as an individual gpio-controller node. ..."
If this is not a good approach, could you please me point me out to a
device tree example where
the correct approach is being used?
Thanks in advance.
Best regards,
Sergio Paracuellos
>> +
>> +Example:
>> + gpio@600 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + compatible = "mediatek,mt7621-gpio";
>> + reg = <0x600 0x100>;
>> +
>> + interrupt-parent = <>;
>> + interrupts = ;
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> +
>> + gpio0: bank@0 {
>> + reg = <0>;
>> + compatible = "mediatek,mt7621-gpio-bank";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> +
>> + gpio1: bank@1 {
>> + reg = <1>;
>> + compatible = "mediatek,mt7621-gpio-bank";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> +
>> + gpio2: bank@2 {
>> + reg = <2>;
>> + compatible = "mediatek,mt7621-gpio-bank";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> + };
>> --
>> 2.7.4
>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/3] hwmon: (nct7904) Fix style issues
On Wed, Jun 13, 2018 at 05:13:22PM +0200, Jakob Albert wrote: > This set of patches fixes style errors reported by checkpatch.pl. This > adapts the code to the coding style. > > Changes since v1: > * Changed patch subjects and descriptions > > > > Jakob Albert (3): > hwmon: (nct7904) Fix SPACING errors > hwmon: (nct7904) Fix CODE_INDENT error > hwmon: (nct7904) Fix UNSPECIFIED_INT warning > Series applied to hwmon-next. Thanks, Guenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/3] hwmon: (nct7904) Fix UNSPECIFIED_INT warning
Fix UNSPECIFIED_INT warning reported by checkpatch.pl
Signed-off-by: Lorenz Kaestle
Signed-off-by: Jakob Albert
---
v1->v2: Changed patch subject and description
drivers/hwmon/nct7904.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
index 657ce88..7815ddf 100644
--- a/drivers/hwmon/nct7904.c
+++ b/drivers/hwmon/nct7904.c
@@ -77,7 +77,7 @@ struct nct7904_data {
};
/* Access functions */
-static int nct7904_bank_lock(struct nct7904_data *data, unsigned bank)
+static int nct7904_bank_lock(struct nct7904_data *data, unsigned int bank)
{
int ret;
@@ -99,7 +99,7 @@ static inline void nct7904_bank_release(struct nct7904_data
*data)
/* Read 1-byte register. Returns unsigned reg or -ERRNO on error. */
static int nct7904_read_reg(struct nct7904_data *data,
- unsigned bank, unsigned reg)
+ unsigned int bank, unsigned int reg)
{
struct i2c_client *client = data->client;
int ret;
@@ -117,7 +117,7 @@ static int nct7904_read_reg(struct nct7904_data *data,
* -ERRNO on error.
*/
static int nct7904_read_reg16(struct nct7904_data *data,
- unsigned bank, unsigned reg)
+ unsigned int bank, unsigned int reg)
{
struct i2c_client *client = data->client;
int ret, hi;
@@ -139,7 +139,7 @@ static int nct7904_read_reg16(struct nct7904_data *data,
/* Write 1-byte register. Returns 0 or -ERRNO on error. */
static int nct7904_write_reg(struct nct7904_data *data,
-unsigned bank, unsigned reg, u8 val)
+unsigned int bank, unsigned int reg, u8 val)
{
struct i2c_client *client = data->client;
int ret;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/3] hwmon: (nct7904) Fix CODE_INDENT error
Fix CODE_INDENT error reported by checkpatch.pl
Signed-off-by: Lorenz Kaestle
Signed-off-by: Jakob Albert
---
v1->v2: Changed patch subject and description
drivers/hwmon/nct7904.c | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
index 7de2421..657ce88 100644
--- a/drivers/hwmon/nct7904.c
+++ b/drivers/hwmon/nct7904.c
@@ -431,15 +431,15 @@ static const struct hwmon_channel_info nct7904_in = {
};
static const u32 nct7904_fan_config[] = {
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
-HWMON_F_INPUT,
- 0
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ 0
};
static const struct hwmon_channel_info nct7904_fan = {
@@ -448,11 +448,11 @@ static const struct hwmon_channel_info nct7904_fan = {
};
static const u32 nct7904_pwm_config[] = {
-HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
- 0
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ 0
};
static const struct hwmon_channel_info nct7904_pwm = {
@@ -461,16 +461,16 @@ static const struct hwmon_channel_info nct7904_pwm = {
};
static const u32 nct7904_temp_config[] = {
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
-HWMON_T_INPUT,
- 0
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ 0
};
static const struct hwmon_channel_info nct7904_temp = {
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/3] hwmon: (nct7904) Fix SPACING errors
Fix SPACING errors reported by checkpatch.pl
Signed-off-by: Lorenz Kaestle
Signed-off-by: Jakob Albert
---
v1->v2: Changed patch subject and description
drivers/hwmon/nct7904.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
index 95a68ab..7de2421 100644
--- a/drivers/hwmon/nct7904.c
+++ b/drivers/hwmon/nct7904.c
@@ -159,7 +159,7 @@ static int nct7904_read_fan(struct device *dev, u32 attr,
int channel,
unsigned int cnt, rpm;
int ret;
- switch(attr) {
+ switch (attr) {
case hwmon_fan_input:
ret = nct7904_read_reg16(data, BANK_0,
FANIN1_HV_REG + channel * 2);
@@ -200,7 +200,7 @@ static int nct7904_read_in(struct device *dev, u32 attr,
int channel,
index = nct7904_chan_to_index[channel];
- switch(attr) {
+ switch (attr) {
case hwmon_in_input:
ret = nct7904_read_reg16(data, BANK_0,
VSEN1_HV_REG + index * 2);
@@ -236,7 +236,7 @@ static int nct7904_read_temp(struct device *dev, u32 attr,
int channel,
struct nct7904_data *data = dev_get_drvdata(dev);
int ret, temp;
- switch(attr) {
+ switch (attr) {
case hwmon_temp_input:
if (channel == 0)
ret = nct7904_read_reg16(data, BANK_0, LTD_HV_REG);
@@ -276,7 +276,7 @@ static int nct7904_read_pwm(struct device *dev, u32 attr,
int channel,
struct nct7904_data *data = dev_get_drvdata(dev);
int ret;
- switch(attr) {
+ switch (attr) {
case hwmon_pwm_input:
ret = nct7904_read_reg(data, BANK_3, FANCTL1_OUT_REG + channel);
if (ret < 0)
@@ -301,7 +301,7 @@ static int nct7904_write_pwm(struct device *dev, u32 attr,
int channel,
struct nct7904_data *data = dev_get_drvdata(dev);
int ret;
- switch(attr) {
+ switch (attr) {
case hwmon_pwm_input:
if (val < 0 || val > 255)
return -EINVAL;
@@ -322,7 +322,7 @@ static int nct7904_write_pwm(struct device *dev, u32 attr,
int channel,
static umode_t nct7904_pwm_is_visible(const void *_data, u32 attr, int channel)
{
- switch(attr) {
+ switch (attr) {
case hwmon_pwm_input:
case hwmon_pwm_enable:
return S_IRUGO | S_IWUSR;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/3] hwmon: (nct7904) Fix style issues
This set of patches fixes style errors reported by checkpatch.pl. This adapts the code to the coding style. Changes since v1: * Changed patch subjects and descriptions Jakob Albert (3): hwmon: (nct7904) Fix SPACING errors hwmon: (nct7904) Fix CODE_INDENT error hwmon: (nct7904) Fix UNSPECIFIED_INT warning drivers/hwmon/nct7904.c | 68 - 1 file changed, 34 insertions(+), 34 deletions(-) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
On 12/06/18 22:09, Chris Opperman wrote:
Changes since v3:
a) Reverted u64 to unsigned long long and u32 to unsigned int.
b) Added patch versioning.
c) Changed type of scans_left to unsigned long long to avoid cast.
d) Clarified and updated changelog.
8---8<
Improve readability of comedi_nsamples_left:
a) Reduce nesting by using more return statements.
b) Declare variables scans_left and samples_left at start of function.
c) Change type of scans_Left to unsigned long long to avoid cast.
Signed-off-by: Chris Opperman
---
drivers/staging/comedi/drivers.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..57dd63d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -473,21 +473,21 @@ unsigned int comedi_nsamples_left(struct comedi_subdevice
*s,
{
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = >cmd;
+ unsigned long long scans_left;
+ unsigned long long samples_left;
- if (cmd->stop_src == TRIG_COUNT) {
- unsigned int scans_left = __comedi_nscans_left(s,
cmd->stop_arg);
- unsigned int scan_pos =
- comedi_bytes_to_samples(s, async->scan_progress);
- unsigned long long samples_left = 0;
-
- if (scans_left) {
- samples_left = ((unsigned long long)scans_left *
- cmd->scan_end_arg) - scan_pos;
- }
+ if (cmd->stop_src != TRIG_COUNT)
+ return nsamples;
- if (samples_left < nsamples)
- nsamples = samples_left;
- }
+ scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+ if (!scans_left)
+ return 0;
+
+ samples_left = scans_left * cmd->scan_end_arg -
+ comedi_bytes_to_samples(s, async->scan_progress);
+
+ if (samples_left < nsamples)
+ return samples_left;
return nsamples;
}
EXPORT_SYMBOL_GPL(comedi_nsamples_left);
--
2.1.4
The code changes look fine. You just need to correct the commit
message, as pointed out by Dan Carpenter.
For reference, everything below the "---" cut-off line gets stripped out
of the commit message in the git repository, so that is a good place to
add comments about the patch itself (such as change logs) that do not
belong in the commit message.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging: lustre: add error handling for try_module_get
On Wed, Jun 13 2018, David Laight wrote:
> From: Zhouyang Jia
>> Sent: 12 June 2018 05:49
>>
>> When try_module_get fails, the lack of error-handling code may
>> cause unexpected results.
>>
>> This patch adds error-handling code after calling try_module_get.
> ...
>> +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c
>> @@ -2422,7 +2422,10 @@ ksocknal_base_startup(void)
>>
>> /* flag lists/ptrs/locks initialised */
>> ksocknal_data.ksnd_init = SOCKNAL_INIT_DATA;
>> -try_module_get(THIS_MODULE);
>> +if (!try_module_get(THIS_MODULE)) {
>> +CERROR("%s: cannot get module\n", __func__);
>> +goto failed;
>> +}
>
>
> Can try_module_get(THIS_MODULE) ever fail?
Yes.
> Since you are running code in 'THIS_MODULE' the caller must have a
> reference that can't go away.
Not necessarily, though it does usually work that way.
try_module_get() can fail while the exit function is running, but it is
safe to run code in the module until the exit function completes.
So if the exit function takes a lock, then other code can safely run
code in the module while holding the lock, but not holding a reference
to the module. If this code calls try_module_get(), it could fail.
That is exactly what is happening here.
ksoclnd_exit() calls lnet_unregister_lnd() which takes
the_lnet.ln_lnd_mutex.
ksocknal_base_startup() is called from ksocknal_startup()
which is the_ksocklnd.lnd_startup and is called, from
lnet_startup_lndni(), with that lock held.
> So try_module_get() just increments the count that is already greater
> than zero.
>
> Similarly module_put(THIS_MODULE) must never be able to release the
> last reference.
It can if a suitable lock is held.
> Any such calls that aren't in error paths after try_module_get() are
> probably buggy.
Being in an error path doesn't make it safe.
module_put(THIS_MODULE) can only be safe if a lock is held which
prevents the exit function from completing. Some code outside the
module must release the lock.
Having said that, I don't really like this approach. I much prefer for
the module reference to be taken and put outside of the module - it
seems less error-prone.
NeilBrown
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging: lustre: add error handling for try_module_get
From: Zhouyang Jia
> Sent: 12 June 2018 05:49
>
> When try_module_get fails, the lack of error-handling code may
> cause unexpected results.
>
> This patch adds error-handling code after calling try_module_get.
...
> +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c
> @@ -2422,7 +2422,10 @@ ksocknal_base_startup(void)
>
> /* flag lists/ptrs/locks initialised */
> ksocknal_data.ksnd_init = SOCKNAL_INIT_DATA;
> - try_module_get(THIS_MODULE);
> + if (!try_module_get(THIS_MODULE)) {
> + CERROR("%s: cannot get module\n", __func__);
> + goto failed;
> + }
Can try_module_get(THIS_MODULE) ever fail?
Since you are running code in 'THIS_MODULE' the caller must have a
reference that can't go away.
So try_module_get() just increments the count that is already greater
than zero.
Similarly module_put(THIS_MODULE) must never be able to release the
last reference.
Any such calls that aren't in error paths after try_module_get() are
probably buggy.
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings
On Tue, Jun 12, 2018 at 02:56:38PM -0600, Rob Herring wrote: > Bindings are for h/w, not a driver. > > > Signed-off-by: Sergio Paracuellos > > Reviewed-by: NeilBrown > > Space ^ Pretty sure that was deliberate... Otherwise he's been making that same mistake for over a decade now. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: comedi: Improved readability of function comedi_nsamples_left.
So close... On Tue, Jun 12, 2018 at 11:09:44PM +0200, Chris Opperman wrote: > Changes since v3: > a) Reverted u64 to unsigned long long and u32 to unsigned int. > b) Added patch versioning. > c) Changed type of scans_left to unsigned long long to avoid cast. > d) Clarified and updated changelog. > > >8---8< This part here needs to go ... > > Improve readability of comedi_nsamples_left: > a) Reduce nesting by using more return statements. > b) Declare variables scans_left and samples_left at start of function. > c) Change type of scans_Left to unsigned long long to avoid cast. ^ > > Signed-off-by: Chris Opperman > --- ... down here, under the --- cut off line. > drivers/staging/comedi/drivers.c | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/comedi/drivers.c > b/drivers/staging/comedi/drivers.c regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: pi433: fix race condition in pi433_ioctl
On Tue, Jun 12, 2018 at 09:47:41PM -0400, Hugo Lefeuvre wrote:
> drivers/staging/pi433/pi433_if.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index b061f77dda41..3ec1ed01d04b 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -880,6 +880,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned
> long arg)
> int retval = 0;
> struct pi433_instance *instance;
> struct pi433_device *device;
> + struct pi433_tx_cfg tx_cfg_buffer;
> void __user *argp = (void __user *)arg;
>
> /* Check type and command number */
> @@ -902,9 +903,15 @@ pi433_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> return -EFAULT;
> break;
> case PI433_IOC_WR_TX_CFG:
> - if (copy_from_user(>tx_cfg, argp,
> -sizeof(struct pi433_tx_cfg)))
> + /* do not modify tx config while it is being copied to fifo */
There is no need for this comment, since it's obvious. Also if you use
simpler names then the copy fits on one line:
if (copy_from_user(_cfg, argp, sizeof(tx_cfg)) {
> + mutex_lock(>tx_fifo_lock);
> + if (copy_from_user(_cfg_buffer, argp,
> +sizeof(struct pi433_tx_cfg))) {
Sorry for the duplicate review, but it got sent to both my inboxes... :P
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: pi433: fix race condition in pi433_ioctl
On Tue, Jun 12, 2018 at 09:47:41PM -0400, Hugo Lefeuvre wrote:
> In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is
> modified via
>
> copy_from_user(>tx_cfg, argp, sizeof(struct pi433_tx_cfg)))
>
> without any kind of synchronization. In the case where two threads
> would execute this same command concurrently the tx_cfg field might
> enter in an inconsistent state.
>
> Additionally: if ioctl(PI433_IOC_WR_TX_CFG) and write() execute
> concurrently the tx config might be modified while it is being
> copied to the fifo, resulting in potential data corruption.
>
> Fix: Get instance->tx_cfg_lock before modifying tx config in the
> PI433_IOC_WR_TX_CFG case in pi433_ioctl.
>
> Also, do not copy data directly from user space to instance->tx_cfg.
> Instead use a temporary buffer allowing future checks for correctness
> of copied data.
>
> Signed-off-by: Hugo Lefeuvre
> ---
> Changes in v2:
> - Use device->tx_fifo_lock instead of introducing a new lock in
> instance.
> - Do not copy data directly from user space to instance->tx_cfg,
> instead use a temporary buffer allowing future checks for
> correctness of copied data.
> ---
> drivers/staging/pi433/pi433_if.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index b061f77dda41..3ec1ed01d04b 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -880,6 +880,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned
> long arg)
> int retval = 0;
> struct pi433_instance *instance;
> struct pi433_device *device;
> + struct pi433_tx_cfg tx_cfg_buffer;
> void __user *argp = (void __user *)arg;
>
> /* Check type and command number */
> @@ -902,9 +903,15 @@ pi433_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> return -EFAULT;
> break;
> case PI433_IOC_WR_TX_CFG:
> - if (copy_from_user(>tx_cfg, argp,
> -sizeof(struct pi433_tx_cfg)))
> + /* do not modify tx config while it is being copied to fifo */
> + mutex_lock(>tx_fifo_lock);
> + if (copy_from_user(_cfg_buffer, argp,
> +sizeof(struct pi433_tx_cfg))) {
> + mutex_unlock(>tx_fifo_lock);
> return -EFAULT;
> + }
> + memcpy(>tx_cfg, _cfg_buffer, sizeof(struct
> pi433_tx_cfg));
> + mutex_unlock(>tx_fifo_lock);
The lock is only needed around the memcpy() and that makes the code a
bit simpler as well.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 2/4] resource: Use list_head to link sibling resource
On 06/12/18 at 05:10pm, Julia Lawall wrote:
> This looks wrong. After a list iterator, the index variable points to a
> dummy structure.
>
> julia
>
> url:
> https://github.com/0day-ci/linux/commits/Baoquan-He/resource-Use-list_head-to-link-sibling-resource/20180612-113600
> :: branch date: 7 hours ago
> :: commit date: 7 hours ago
>
> >> kernel/resource.c:265:17-20: ERROR: invalid reference to the index
> >> variable of the iterator on line 253
>
> #
> https://github.com/0day-ci/linux/commit/e906f15906750a86913ba2b1f08bad99129d3dfc
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout e906f15906750a86913ba2b1f08bad99129d3dfc
> vim +265 kernel/resource.c
>
> ^1da177e4 Linus Torvalds 2005-04-16 247
> 5eeec0ec9 Yinghai Lu 2009-12-22 248 static void
> __release_child_resources(struct resource *r)
> 5eeec0ec9 Yinghai Lu 2009-12-22 249 {
> e906f1590 Baoquan He 2018-06-12 250 struct resource *tmp, *next;
> 5eeec0ec9 Yinghai Lu 2009-12-22 251 resource_size_t size;
> 5eeec0ec9 Yinghai Lu 2009-12-22 252
> e906f1590 Baoquan He 2018-06-12 @253 list_for_each_entry_safe(tmp,
> next, >child, sibling) {
> 5eeec0ec9 Yinghai Lu 2009-12-22 254 tmp->parent = NULL;
> e906f1590 Baoquan He 2018-06-12 255
> INIT_LIST_HEAD(>sibling);
list_del_init(>sibling);
Thanks, Julia. Here I should use list_del_init(>list) to
replace INIT_LIST_HEAD(>sibling).
> 5eeec0ec9 Yinghai Lu 2009-12-22 256
> __release_child_resources(tmp);
> 5eeec0ec9 Yinghai Lu 2009-12-22 257
> 5eeec0ec9 Yinghai Lu 2009-12-22 258 printk(KERN_DEBUG
> "release child resource %pR\n", tmp);
> 5eeec0ec9 Yinghai Lu 2009-12-22 259 /* need to restore
> size, and keep flags */
> 5eeec0ec9 Yinghai Lu 2009-12-22 260 size =
> resource_size(tmp);
> 5eeec0ec9 Yinghai Lu 2009-12-22 261 tmp->start = 0;
> 5eeec0ec9 Yinghai Lu 2009-12-22 262 tmp->end = size - 1;
> 5eeec0ec9 Yinghai Lu 2009-12-22 263 }
> e906f1590 Baoquan He 2018-06-12 264
> e906f1590 Baoquan He 2018-06-12 @265 INIT_LIST_HEAD(>child);
> 5eeec0ec9 Yinghai Lu 2009-12-22 266 }
> 5eeec0ec9 Yinghai Lu 2009-12-22 267
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
