[PATCH 1/1] staging: rtl8723au: Fix sparse warnings invalid assignment |=
This is a patch to the hal/rtl8723au_xmit.c file that fixes various
warnings:
"invalid warning: invalid assignment: |=
left side has type unsigned int
right side has type restricted __le32"
found by sparse tool.
Signed-off-by: Yannis Damigos
---
drivers/staging/rtl8723au/hal/rtl8723au_xmit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723au/hal/rtl8723au_xmit.c
b/drivers/staging/rtl8723au/hal/rtl8723au_xmit.c
index 6070510..e7e796e 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723au_xmit.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723au_xmit.c
@@ -79,7 +79,7 @@ static void fill_txdesc_sectype(struct pkt_attrib *pattrib,
struct tx_desc *ptxd
}
}
-static void fill_txdesc_vcs(struct pkt_attrib *pattrib, u32 *pdw)
+static void fill_txdesc_vcs(struct pkt_attrib *pattrib, __le32 *pdw)
{
/* DBG_8723A("cvs_mode =%d\n", pattrib->vcs_mode); */
--
2.1.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code
> -Original Message- > From: Michal Hocko [mailto:mho...@suse.cz] > Sent: Thursday, December 11, 2014 4:58 AM > To: KY Srinivasan > Cc: Yasuaki Ishimatsu; gre...@linuxfoundation.org; linux- > ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; > a...@canonical.com; linux...@kvack.org > Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the > memory hot-add code > > On Thu 11-12-14 00:21:09, KY Srinivasan wrote: > > > > > > > -Original Message- > > > From: Michal Hocko [mailto:mho...@suse.cz] > > > Sent: Tuesday, December 9, 2014 2:56 AM > > > To: Yasuaki Ishimatsu > > > Cc: KY Srinivasan; gre...@linuxfoundation.org; linux- > > > ker...@vger.kernel.org; de...@linuxdriverproject.org; > > > o...@aepfle.de; a...@canonical.com; linux...@kvack.org > > > Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock > > > issue in the memory hot-add code > > > > > > On Tue 09-12-14 19:25:50, Yasuaki Ishimatsu wrote: > > > > (2014/12/09 18:08), Michal Hocko wrote: > > > [...] > > > > >Doesn't udev retry the operation if it gets EBUSY or EAGAIN? > > > > > > > > It depend on implementation of udev.rules. So we can retry > > > > online/offline operation in udev.rules. > > > [...] > > > > > > # Memory hotadd request > > > SUBSYSTEM=="memory", ACTION=="add", > > > DEVPATH=="/devices/system/memory/memory*[0-9]", > > > TEST=="/sys$devpath/state", RUN+="/bin/sh -c 'echo online > > > > /sys$devpath/state'" > > > > > > OK so this is not prepared for a temporary failures and retries. > > > > > > > >And again, why cannot we simply make the onlining fail or > > > > >try_lock and retry internally if the event consumer cannot cope with > errors? > > > > > > > > Did you mean the following Srinivasan's first patch looks good to you? > > > > https://lkml.org/lkml/2014/12/2/662 > > > > > > Heh, I was just about to post this. Because I haven't noticed the > > > previous patch yet. Yeah, Something like that. Except that I would > > > expect EAGAIN or EBUSY rather than ERESTARTSYS which should never > > > leak into userspace. And that would happen here AFAICS because > > > signal_pending will not be true usually. > > Michal, > > > > I agree that the fix to this problem must be outside the clients of > > add_memory() and that is the reason I had sent that patch: > > https://lkml.org/lkml/2014/12/2/662. Let me know if you want me to > > resend this patch with the correct return value. > > Please think about the other suggested options as well. Thanks Michal. I will look at the other options you have listed as well. K. Y > > > Regards, > > > > K. Y > > > > > > So there are two options. Either make the udev rule more robust and > > > retry within RUN section or do the retry withing online_pages > > > (try_lock and go into interruptible sleep which gets signaled by > > > finished add_memory()). The later option is safer wrt. the userspace > > > because the operation wouldn't fail unexpectedly. > > > Another option would be generating the sysfs file after all the > > > internal initialization is done and call it outside of the memory hotplug > lock. > > > > > > -- > > > Michal Hocko > > > SUSE Labs > > -- > Michal Hocko > SUSE Labs ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
.../lustre/lustre/obdclass/lprocfs_status.c| 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..4a7891a 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char *buffer,
unsigned long count,
}
units = 1;
- switch (*end) {
- case 'p': case 'P':
- units <<= 10;
- case 't': case 'T':
- units <<= 10;
- case 'g': case 'G':
- units <<= 10;
- case 'm': case 'M':
- units <<= 10;
- case 'k': case 'K':
- units <<= 10;
+ if (end) {
+ switch (*end) {
+ case 'p': case 'P':
+ case 't': case 'T':
+ case 'g': case 'G':
+ case 'm': case 'M':
+ case 'k': case 'K':
+ units <<= 10;
+ }
}
/* Specified units override the multiplier */
if (units)
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: lustre: osc: lproc_osc.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
drivers/staging/lustre/lustre/osc/lproc_osc.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c
b/drivers/staging/lustre/lustre/osc/lproc_osc.c
index 9f719bc..9ba6293 100644
--- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
+++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
@@ -237,13 +237,15 @@ static ssize_t osc_cur_grant_bytes_seq_write(struct file
*file, const char *buff
size_t count, loff_t *off)
{
struct obd_device *obd = ((struct seq_file
*)file->private_data)->private;
- struct client_obd *cli = &obd->u.cli;
+ struct client_obd *cli;
int rc;
__u64 val;
if (obd == NULL)
return 0;
+ cli = &obd->u.cli;
+
rc = lprocfs_write_u64_helper(buffer, count, &val);
if (rc)
return rc;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723au: os_dep: usb_intf.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
drivers/staging/rtl8723au/os_dep/usb_intf.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723au/os_dep/usb_intf.c
b/drivers/staging/rtl8723au/os_dep/usb_intf.c
index 865743e..71a6330 100644
--- a/drivers/staging/rtl8723au/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8723au/os_dep/usb_intf.c
@@ -351,10 +351,11 @@ error_exit:
int rtw_hw_resume23a(struct rtw_adapter *padapter)
{
struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
- struct net_device *pnetdev = padapter->pnetdev;
+ struct net_device *pnetdev;
if (padapter) { /* system resume */
DBG_8723A("==> rtw_hw_resume23a\n");
+ pnetdev = padapter->pnetdev;
down(&pwrpriv->lock);
pwrpriv->bips_processing = true;
rtw_reset_drv_sw23a(padapter);
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: unisys: virtpci: virtpci.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
drivers/staging/unisys/virtpci/virtpci.c | 23 +--
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index ee9f826..d818eda 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1314,15 +1314,13 @@ static ssize_t virtpci_driver_attr_show(struct kobject
*kobj,
ssize_t ret = 0;
struct driver_private *dprivate = to_driver(kobj);
- struct device_driver *driver;
- if (dprivate != NULL)
- driver = dprivate->driver;
- else
- driver = NULL;
+ if (dprivate) {
+ struct device_driver *driver = dprivate->driver;
+
+ DBGINF("In virtpci_driver_attr_show driver->name:%s\n",
+ driver->name);
- DBGINF("In virtpci_driver_attr_show driver->name:%s\n", driver->name);
- if (driver) {
if (dattr->show)
ret = dattr->show(driver, buf);
}
@@ -1337,16 +1335,13 @@ static ssize_t virtpci_driver_attr_store(struct kobject
*kobj,
ssize_t ret = 0;
struct driver_private *dprivate = to_driver(kobj);
- struct device_driver *driver;
- if (dprivate != NULL)
- driver = dprivate->driver;
- else
- driver = NULL;
+ if (dprivate) {
+ struct device_driver *driver = dprivate->driver;
- DBGINF("In virtpci_driver_attr_store driver->name:%s\n", driver->name);
+ DBGINF("In virtpci_driver_attr_store driver->name:%s\n",
+ driver->name);
- if (driver) {
if (dattr->store)
ret = dattr->store(driver, buf, count);
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vt6655: wmgr.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
drivers/staging/vt6655/wmgr.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vt6655/wmgr.c b/drivers/staging/vt6655/wmgr.c
index c73c39d..f98935c 100644
--- a/drivers/staging/vt6655/wmgr.c
+++ b/drivers/staging/vt6655/wmgr.c
@@ -4391,7 +4391,7 @@ bAdd_PMKID_Candidate(
unsigned int ii = 0;
pr_debug("bAdd_PMKID_Candidate START: (%d)\n",
-(int)pDevice->gsPMKIDCandidate.NumCandidates);
+ (int)(pDevice ? pDevice->gsPMKIDCandidate.NumCandidates : 0));
if ((pDevice == NULL) || (pbyBSSID == NULL) || (psRSNCapObj == NULL))
return false;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
On Sun, Dec 14, 2014 at 11:36:22PM +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
>
> Was largely found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist
> ---
> .../lustre/lustre/obdclass/lprocfs_status.c| 20
> +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index 61e04af..4a7891a 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char *buffer,
> unsigned long count,
> }
>
> units = 1;
> - switch (*end) {
> - case 'p': case 'P':
> - units <<= 10;
> - case 't': case 'T':
> - units <<= 10;
> - case 'g': case 'G':
> - units <<= 10;
> - case 'm': case 'M':
> - units <<= 10;
> - case 'k': case 'K':
> - units <<= 10;
> + if (end) {
> + switch (*end) {
> + case 'p': case 'P':
> + case 't': case 'T':
> + case 'g': case 'G':
> + case 'm': case 'M':
> + case 'k': case 'K':
> + units <<= 10;
> + }
You know you just changed the logic in the code, right?
Why? Have you tested this?
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: virtpci: virtpci.c: Fix for possible null pointer dereference
On Sun, Dec 14, 2014 at 11:41:11PM +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
>
> Was largely found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/unisys/virtpci/virtpci.c | 23 +--
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/unisys/virtpci/virtpci.c
> b/drivers/staging/unisys/virtpci/virtpci.c
> index ee9f826..d818eda 100644
> --- a/drivers/staging/unisys/virtpci/virtpci.c
> +++ b/drivers/staging/unisys/virtpci/virtpci.c
> @@ -1314,15 +1314,13 @@ static ssize_t virtpci_driver_attr_show(struct
> kobject *kobj,
> ssize_t ret = 0;
>
> struct driver_private *dprivate = to_driver(kobj);
> - struct device_driver *driver;
>
> - if (dprivate != NULL)
> - driver = dprivate->driver;
> - else
> - driver = NULL;
> + if (dprivate) {
It is impossible for dprivate to ever be NULL, someone doesn't
understand how kobjects work :(
So just remove the check please, don't add more logic that will never
run.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
Sorry! extremely stupid. Sending new patch immediately.
Kind regards
Rickard Strandqvist
2014-12-14 23:39 GMT+01:00 Greg Kroah-Hartman :
> On Sun, Dec 14, 2014 at 11:36:22PM +0100, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist
>> ---
>> .../lustre/lustre/obdclass/lprocfs_status.c| 20
>> +---
>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> index 61e04af..4a7891a 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char
>> *buffer, unsigned long count,
>> }
>>
>> units = 1;
>> - switch (*end) {
>> - case 'p': case 'P':
>> - units <<= 10;
>> - case 't': case 'T':
>> - units <<= 10;
>> - case 'g': case 'G':
>> - units <<= 10;
>> - case 'm': case 'M':
>> - units <<= 10;
>> - case 'k': case 'K':
>> - units <<= 10;
>> + if (end) {
>> + switch (*end) {
>> + case 'p': case 'P':
>> + case 't': case 'T':
>> + case 'g': case 'G':
>> + case 'm': case 'M':
>> + case 'k': case 'K':
>> + units <<= 10;
>> + }
>
> You know you just changed the logic in the code, right?
>
> Why? Have you tested this?
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist
---
.../lustre/lustre/obdclass/lprocfs_status.c| 24 +++-
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..5227324 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer,
unsigned long count,
}
units = 1;
- switch (*end) {
- case 'p': case 'P':
- units <<= 10;
- case 't': case 'T':
- units <<= 10;
- case 'g': case 'G':
- units <<= 10;
- case 'm': case 'M':
- units <<= 10;
- case 'k': case 'K':
- units <<= 10;
+ if (end) {
+ switch (*end) {
+ case 'p': case 'P':
+ units <<= 10;
+ case 't': case 'T':
+ units <<= 10;
+ case 'g': case 'G':
+ units <<= 10;
+ case 'm': case 'M':
+ units <<= 10;
+ case 'k': case 'K':
+ units <<= 10;
+ }
}
/* Specified units override the multiplier */
if (units)
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vt6655: wmgr.c: Fix for possible null pointer dereference
Hi
Ok, if you want, otherwise we can put it after the NULL check?
Kind regards
Rickard Strandqvist
2014-12-14 23:52 GMT+01:00 Joe Perches :
> On Sun, 2014-12-14 at 23:42 +0100, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>> Was largely found by using a static code analysis program called cppcheck.
> []
>> diff --git a/drivers/staging/vt6655/wmgr.c b/drivers/staging/vt6655/wmgr.c
> []
>> @@ -4391,7 +4391,7 @@ bAdd_PMKID_Candidate(
>> unsigned int ii = 0;
>>
>> pr_debug("bAdd_PMKID_Candidate START: (%d)\n",
>> - (int)pDevice->gsPMKIDCandidate.NumCandidates);
>> + (int)(pDevice ? pDevice->gsPMKIDCandidate.NumCandidates : 0));
>
> This is a function tracing printk.
> I think it's better to just remove it.
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: unisys: virtpci: virtpci.c: Removes unnecessary NULL check
It is impossible for dprivate to ever be NULL, no check required.
Signed-off-by: Rickard Strandqvist
---
drivers/staging/unisys/virtpci/virtpci.c | 29 +
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index ee9f826..37b94de 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1314,18 +1314,13 @@ static ssize_t virtpci_driver_attr_show(struct kobject
*kobj,
ssize_t ret = 0;
struct driver_private *dprivate = to_driver(kobj);
- struct device_driver *driver;
+ struct device_driver *driver = dprivate->driver;
- if (dprivate != NULL)
- driver = dprivate->driver;
- else
- driver = NULL;
+ DBGINF("In virtpci_driver_attr_show driver->name:%s\n", driver->name);
+
+ if (dattr->show)
+ ret = dattr->show(driver, buf);
- DBGINF("In virtpci_driver_attr_show driver->name:%s\n", driver->name);
- if (driver) {
- if (dattr->show)
- ret = dattr->show(driver, buf);
- }
return ret;
}
@@ -1337,19 +1332,13 @@ static ssize_t virtpci_driver_attr_store(struct kobject
*kobj,
ssize_t ret = 0;
struct driver_private *dprivate = to_driver(kobj);
- struct device_driver *driver;
-
- if (dprivate != NULL)
- driver = dprivate->driver;
- else
- driver = NULL;
+ struct device_driver *driver = dprivate->driver;
DBGINF("In virtpci_driver_attr_store driver->name:%s\n", driver->name);
- if (driver) {
- if (dattr->store)
- ret = dattr->store(driver, buf, count);
- }
+ if (dattr->store)
+ ret = dattr->store(driver, buf, count);
+
return ret;
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vt6655: wmgr.c: Fix for possible null pointer dereference
On Sun, 2014-12-14 at 23:42 +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> Was largely found by using a static code analysis program called cppcheck.
[]
> diff --git a/drivers/staging/vt6655/wmgr.c b/drivers/staging/vt6655/wmgr.c
[]
> @@ -4391,7 +4391,7 @@ bAdd_PMKID_Candidate(
> unsigned int ii = 0;
>
> pr_debug("bAdd_PMKID_Candidate START: (%d)\n",
> - (int)pDevice->gsPMKIDCandidate.NumCandidates);
> + (int)(pDevice ? pDevice->gsPMKIDCandidate.NumCandidates : 0));
This is a function tracing printk.
I think it's better to just remove it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
On Sun, 2014-12-14 at 23:52 +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
>
> Was largely found by using a static code analysis program called cppcheck.
Perhaps the tool could use a little work.
It's not possible for end to be NULL no?
unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int
base)
{
unsigned long long result;
unsigned int rv;
cp = _parse_integer_fixup_radix(cp, &base);
rv = _parse_integer(cp, base, &result);
/* FIXME */
cp += (rv & ~KSTRTOX_OVERFLOW);
if (endp)
*endp = (char *)cp;
return result;
}
EXPORT_SYMBOL(simple_strtoull);
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
[]
Above this:
whole = simple_strtoull(pbuf, &end, 10);
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer,
> unsigned long count,
> }
>
> units = 1;
> - switch (*end) {
> - case 'p': case 'P':
> - units <<= 10;
> - case 't': case 'T':
> - units <<= 10;
> - case 'g': case 'G':
> - units <<= 10;
> - case 'm': case 'M':
> - units <<= 10;
> - case 'k': case 'K':
> - units <<= 10;
> + if (end) {
> + switch (*end) {
> + case 'p': case 'P':
> + units <<= 10;
> + case 't': case 'T':
> + units <<= 10;
> + case 'g': case 'G':
> + units <<= 10;
> + case 'm': case 'M':
> + units <<= 10;
> + case 'k': case 'K':
> + units <<= 10;
> + }
The only thing I might do is
switch (tolower(*end)) {
and remove the second case entry for each line
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: wlan-ng: hfa384x_usb: fixed an 'else' statement coding style issue
Fixed a coding style issue
Signed-off-by: Eduardo Barretto
---
drivers/staging/wlan-ng/hfa384x_usb.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c
b/drivers/staging/wlan-ng/hfa384x_usb.c
index 55d2f56..d388f2c 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -4123,12 +4123,11 @@ static int hfa384x_isgood_pdrcode(u16 pdrcode)
pr_debug("Encountered unknown PDR#=0x%04x, assuming
it's ok.\n",
pdrcode);
return 1;
- } else {
- /* bad code */
- pr_debug("Encountered unknown PDR#=0x%04x, (>=0x1000),
assuming it's bad.\n",
-pdrcode);
- return 0;
}
+ /* bad code */
+ pr_debug("Encountered unknown PDR#=0x%04x, (>=0x1000), assuming
it's bad.\n",
+pdrcode);
+ return 0;
}
return 0; /* avoid compiler warnings */
}
--
2.1.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: wlan-ng: hfa384x_usb: fixed an 'else' statement coding style issue
On Mon, 2014-12-15 at 00:02 -0200, Eduardo Barretto wrote:
> Fixed a coding style issue
[]
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c
> b/drivers/staging/wlan-ng/hfa384x_usb.c
[]
> @@ -4123,12 +4123,11 @@ static int hfa384x_isgood_pdrcode(u16 pdrcode)
> pr_debug("Encountered unknown PDR#=0x%04x, assuming
> it's ok.\n",
>pdrcode);
> return 1;
> - } else {
> - /* bad code */
> - pr_debug("Encountered unknown PDR#=0x%04x, (>=0x1000),
> assuming it's bad.\n",
> - pdrcode);
> - return 0;
> }
> + /* bad code */
> + pr_debug("Encountered unknown PDR#=0x%04x, (>=0x1000), assuming
> it's bad.\n",
> + pdrcode);
> + return 0;
> }
> return 0; /* avoid compiler warnings */
> }
While this patch isn't necessary, if any change is
done, it might better to not have two consecutive
return 0; uses.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] Drivers: hv: vmbus: Use get_cpu() to get the current CPU
Replace calls for smp_processor_id() to get_cpu() to get the CPU ID of
the current CPU. In these instances, there is no correctness issue with
regards to preemption, we just need the current CPU ID.
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/channel_mgmt.c |4 +++-
drivers/hv/connection.c |6 --
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index d36ce68..a205246 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -813,7 +813,7 @@ cleanup:
struct vmbus_channel *vmbus_get_outgoing_channel(struct vmbus_channel *primary)
{
struct list_head *cur, *tmp;
- int cur_cpu = hv_context.vp_index[smp_processor_id()];
+ int cur_cpu;
struct vmbus_channel *cur_channel;
struct vmbus_channel *outgoing_channel = primary;
int cpu_distance, new_cpu_distance;
@@ -821,6 +821,8 @@ struct vmbus_channel *vmbus_get_outgoing_channel(struct
vmbus_channel *primary)
if (list_empty(&primary->sc_list))
return outgoing_channel;
+ cur_cpu = hv_context.vp_index[get_cpu()];
+ put_cpu();
list_for_each_safe(cur, tmp, &primary->sc_list) {
cur_channel = list_entry(cur, struct vmbus_channel, sc_list);
if (cur_channel->state != CHANNEL_OPENED_STATE)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index e206619..a63a795 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -80,8 +80,10 @@ static int vmbus_negotiate_version(struct
vmbus_channel_msginfo *msginfo,
msg->interrupt_page = virt_to_phys(vmbus_connection.int_page);
msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]);
msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]);
- if (version == VERSION_WIN8_1)
- msg->target_vcpu = hv_context.vp_index[smp_processor_id()];
+ if (version == VERSION_WIN8_1) {
+ msg->target_vcpu = hv_context.vp_index[get_cpu()];
+ put_cpu();
+ }
/*
* Add to list before we send the request since we may
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
> -Original Message- > From: Jeremiah Mahler [mailto:jmmah...@gmail.com] > Sent: Wednesday, December 10, 2014 6:10 PM > To: KY Srinivasan > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > jasow...@redhat.com; mc...@ipxe.org > Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in > vmbus_establish_gpadl() > > K. Y. Srinivasan, > > On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote: > > Correctly compute the local (gpadl) handle. > > This description is still too sparse for me. How was it computed before and > why was this incorrect? Pretend like you are trying to explain your patch to > someone who has no idea what you did. > > > I would like to thank Michael Brown for seeing this bug. > > > > Signed-off-by: K. Y. Srinivasan > > Reported-by: Michael Brown > > --- > > Changes in V2: Added the Reported-by tag. > > Changes in V3: Cleaned up the commit log. > > > > drivers/hv/channel.c |4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index > > 433f72a..c76ffbe 100644 > > --- a/drivers/hv/channel.c > > +++ b/drivers/hv/channel.c > > @@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel > *channel, void *kbuffer, > > unsigned long flags; > > int ret = 0; > > > > - next_gpadl_handle = > atomic_read(&vmbus_connection.next_gpadl_handle); > > - atomic_inc(&vmbus_connection.next_gpadl_handle); > > + next_gpadl_handle = > > + > (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); > > > Tell me if I understand this correctly. > > Before it read the handle and incremented it. > > y = x + 1 > > Now it reads the handle, increments it, then decrements it. > > y = (x + 1) - 1 = x This code can be executed concurrently on multiple CPUs. We want to ensure that each call to establish gpadl gets a unique local handle. The earlier code was buggy in that we would read the handle and then atomically increment it. Thus, multiple CPUs could read the identical current value which would be their local handle. What we want is the ability to atomically read and increment the value - this would ensure that each caller got a unique value even if they executed the code concurrently on multiple CPUs. The API atomic_inc_return(), atomically increments and returns the incremented value. We locally decrement this value to emulate the logic of "read the current value and atomically increment the value. Hope this helps, K. Y > > > ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount); > > if (ret) > > -- > > 1.7.4.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > > linux-kernel" in the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > -- > - Jeremiah Mahler ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
KY Srinivasan, On Mon, Dec 15, 2014 at 07:00:45AM +, KY Srinivasan wrote: > > > > -Original Message- > > From: Jeremiah Mahler [mailto:jmmah...@gmail.com] > > Sent: Wednesday, December 10, 2014 6:10 PM > > To: KY Srinivasan > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > > jasow...@redhat.com; mc...@ipxe.org > > Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in > > vmbus_establish_gpadl() > > > > K. Y. Srinivasan, > > > > On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote: > > > Correctly compute the local (gpadl) handle. > > > > This description is still too sparse for me. How was it computed before and > > why was this incorrect? Pretend like you are trying to explain your patch > > to > > someone who has no idea what you did. > > > > > I would like to thank Michael Brown for seeing this bug. > > > > > > Signed-off-by: K. Y. Srinivasan > > > Reported-by: Michael Brown > > > --- > > > Changes in V2: Added the Reported-by tag. > > > Changes in V3: Cleaned up the commit log. > > > > > > drivers/hv/channel.c |4 ++-- > > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index > > > 433f72a..c76ffbe 100644 > > > --- a/drivers/hv/channel.c > > > +++ b/drivers/hv/channel.c > > > @@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel > > *channel, void *kbuffer, > > > unsigned long flags; > > > int ret = 0; > > > > > > - next_gpadl_handle = > > atomic_read(&vmbus_connection.next_gpadl_handle); > > > - atomic_inc(&vmbus_connection.next_gpadl_handle); > > > + next_gpadl_handle = > > > + > > (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); > > > > > Tell me if I understand this correctly. > > > > Before it read the handle and incremented it. > > > > y = x + 1 > > > > Now it reads the handle, increments it, then decrements it. > > > > y = (x + 1) - 1 = x > > This code can be executed concurrently on multiple CPUs. We want to ensure > that each call to > establish gpadl gets a unique local handle. The earlier code was buggy in > that we would read the > handle and then atomically increment it. Thus, multiple CPUs could read the > identical current > value which would be their local handle. What we want is the ability to > atomically read and increment > the value - this would ensure that each caller got a unique value even if > they executed the code > concurrently on multiple CPUs. The API atomic_inc_return(), atomically > increments and returns the > incremented value. We locally decrement this value to emulate the logic of > "read the current value and > atomically increment the value. > > Hope this helps, > > K. Y > > [...] So to avoid concurrency issues you used a single atomic operation instead of two separate operations. That make sense. But it still doesn't explain why you changed the calculation by subtracting 1. -- - Jeremiah Mahler ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
