Re: [PATCH v2] staging: emxx_udc: allow modular build
Hi Linus,
I have been using the following patch as a merge resolution in the
merge of the usb tree for a while now. Greg seems to have missed it
when asking you to merge his tree ... It now applies cleanly to your
tree.
From: Arnd Bergmann
To: kernel-build-repo...@lists.linaro.org
Cc: Mark Brown ,
Greg Kroah-Hartman ,
Magnus Damm ,
Simon Horman ,
linaro-ker...@lists.linaro.org, linux-n...@vger.kernel.org,
linux-...@vger.kernel.org
Subject: [PATCH, RESEND] staging: emxx_udc: allow modular build
Date: Tue, 05 Jul 2016 12:00:49 +0200
Message-ID: <4551753.qTqdp3sOje@wuerfel>
A change to the usb gadget core allowed certain API functions to be
part of a loadable module, which breaks having emxx_udc built-in:
drivers/staging/built-in.o: In function `nbu2ss_drv_probe':
(.text+0x2428): undefined reference to `usb_ep_set_maxpacket_limit'
The original patch already fixed tons of other cases that have the
added dependency but apparently missed this one that now appears
in an ARM allmodconfig build.
This patch makes the symbol "tristate", which lets the Kconfig
dependency tracking handle it correctly. To make the module
actually usable, I also revert 0af61e66ee16 ("drivers/staging:
make emxx_udc.c explicitly non-modular"), which Paul Gortmaker
added after noticing that the Kconfig symbol was 'bool'.
Compared to the original version however, I leave out the
'__exit' annotation on the remove callback, as Paul pointed
out that this was incorrect.
Signed-off-by: Arnd Bergmann
Fixes: 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core")
---
On Tuesday, July 5, 2016 11:52:42 AM CEST Mark Brown wrote:
> On Tue, Jul 05, 2016 at 10:15:03AM +0100, Build bot for Mark Brown wrote:
>
> For the past little while both arm and arm64 allmodconfig builds have
> been failing with:
>
> > drivers/built-in.o: In function `nbu2ss_drv_probe':
> > binder.c:(.text+0x29438c): undefined reference to
> > `usb_ep_set_maxpacket_limit'
> > binder.c:(.text+0x294468): undefined reference to
> > `usb_ep_set_maxpacket_limit'
>
> That function is a static inline in linux/usb/gadget.h which does seem
> to be included (the driver builds fine) so I'm not entirely sure why
> this is failing - I've not had time to investigate properly, I don't
> know if the compiler is misfiring here.
>
This is the patch I sent for it when it first appeared:
https://lkml.org/lkml/2016/6/23/528
diff --git a/drivers/staging/emxx_udc/Kconfig b/drivers/staging/emxx_udc/Kconfig
index cc3402020487..d7577096fb25 100644
--- a/drivers/staging/emxx_udc/Kconfig
+++ b/drivers/staging/emxx_udc/Kconfig
@@ -1,5 +1,5 @@
config USB_EMXX
- bool "EMXX USB Function Device Controller"
+ tristate "EMXX USB Function Device Controller"
depends on USB_GADGET && (ARCH_SHMOBILE || (ARM && COMPILE_TEST))
help
The Emma Mobile series of SoCs from Renesas Electronics and
diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index 3bd91758b2da..3b56b2826263 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -15,7 +15,7 @@
*/
#include
-#include
+#include
#include
#include
#include
@@ -39,9 +39,11 @@
#include "emxx_udc.h"
+#defineDRIVER_DESC "EMXX UDC driver"
#defineDMA_ADDR_INVALID(~(dma_addr_t)0)
static const char driver_name[] = "emxx_udc";
+static const char driver_desc[] = DRIVER_DESC;
/*===*/
/* Prototype */
@@ -3296,6 +3298,28 @@ static void nbu2ss_drv_shutdown(struct platform_device
*pdev)
}
/*-*/
+static int nbu2ss_drv_remove(struct platform_device *pdev)
+{
+ struct nbu2ss_udc *udc;
+ struct nbu2ss_ep*ep;
+ int i;
+
+ udc = &udc_controller;
+
+ for (i = 0; i < NUM_ENDPOINTS; i++) {
+ ep = &udc->ep[i];
+ if (ep->virt_buf)
+ dma_free_coherent(NULL, PAGE_SIZE,
+ (void *)ep->virt_buf, ep->phys_buf);
+ }
+
+ /* Interrupt Handler - Release */
+ free_irq(INT_VBUS, udc);
+
+ return 0;
+}
+
+/*-*/
static int nbu2ss_drv_suspend(struct platform_device *pdev, pm_message_t state)
{
struct nbu2ss_udc *udc;
@@ -3347,12 +3371,16 @@ static int nbu2ss_drv_resume(struct platform_device
*pdev)
static struct platform_driver udc_driver = {
.probe = nbu2ss_drv_probe,
.shutdown = nbu2ss_drv_shutdown,
+ .remove = nbu2ss_drv_remove,
.suspend= nbu2ss_drv_suspend,
.resume = nbu2ss_drv_resume,
.driver = {
-
Re: staging: ks7010: Rename jump labels
Hi Markus, On Thu, 21 Jul 2016 09:55:55 +0200, SF Markus Elfring wrote: > > > How do you think about information from the chapter "7: Centralized > > > exiting of functions"? > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?id=47ef4ad2684d380dd6d596140fb79395115c3950#n389 > > > > I'm not impressed by this piece of documentation. > > I would also appreciate further improvements there. > > > > Back to the lack of space before labels, it's at best a personal preference. > > If you insist on standardizing, I'd call it a bug in the documentation, > > which should be fixed. One space before label is the way to go. > > Would you like to contribute another patch for such a coding style issue? I just sent a patch out, let's see what people think about it. > > That being said... checkpatch does not complain about leading space > > before labels. Not even with --strict. So why are you mentioning it here? > > How do you think about a similar software update? > > staging: lustre: Fix a jump label position in osc_get_info() > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c71d264543f759fea147734cb63de36397817534 > https://lkml.org/lkml/2015/12/21/401 Intending labels with a tab is wrong, so fixing it is welcome. I'd turn the tab into a space instead, for reasons explained before, but no indentation at all is still better than a tab. -- Jean Delvare SUSE L3 Support ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: ks7010: Rename jump labels
Hi Markus, On Thu, 21 Jul 2016 22:11:53 +0200, SF Markus Elfring wrote: > >> How do you generally think about jump label renaming? > > > > Renaming from "out0:", "out1:" etc to something meaningful, yes. > > I suggest to take another look at such identifiers. > > Would you like to support the renaming of a label like "error_out1" > (in the function "ks7010_upload_firmware" for example)? They should be renamed too. Anything using numbers instead of explicit labels should be updated. I included the reasons in the patch I just sent, hopefully the documentation is clearer now. > Will the software evolution be continued also with information from the topic > "Source code review around jump label usage"? > https://lkml.org/lkml/2015/12/11/378 > http://article.gmane.org/gmane.linux.kernel/2106190 Personally I see no value in such statistics. Either labels are wrong (either wrong indentation or wrong name) and should be fixed, or they are correct and you should not touch them. Whether the same label name is used somewhere else is irrelevant. Labels are local by nature, so uniqueness isn't a goal at all, only correctness. -- Jean Delvare SUSE L3 Support ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: ks7010: Rename jump labels
>> Would you like to support the renaming of a label like "error_out1" >> (in the function "ks7010_upload_firmware" for example)? > > They should be renamed too. Anything using numbers instead of explicit Interesting … > Anything using numbers instead of explicit labels should be updated. Would you dare to search for corresponding update candidates explicitly by special semantic patch scripts? > I included the reasons in the patch I just sent, > hopefully the documentation is clearer now. I am curious on how feedback will evolve for your suggestion "CodingStyle: Clarify and complete chapter 7". https://lkml.org/lkml/2016/7/25/207 How do you think about to show a shorter label like "free_bar" (instead of "err_free_bar") as an example? >> "Source code review around jump label usage"? >> https://lkml.org/lkml/2015/12/11/378 >> http://article.gmane.org/gmane.linux.kernel/2106190 > > Personally I see no value in such statistics. Do they indicate any code smells eventually? > Either labels are wrong (either wrong indentation or wrong name) > and should be fixed, or they are correct and you should not touch them. Do you find such changes worthwhile (without touching also any surrounding source code)? Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] PCI: hv: Fix interrupt cleanup path
On Tue, Jul 12, 2016 at 11:31:24AM -0400, Cathy Avery wrote:
> SR-IOV disabled from the host causes a memory leak.
> pci-hyperv usually first receives a PCI_EJECT notification
> and then proceeds to delete the hpdev list entry in
> hv_eject_device_work(). Later in hv_msi_free() since the
> device is no longer on the device list hpdev is NULL
> and hv_msi_free returns without freeing int_desc as part of
> hv_int_desc_free().
>
> Signed-off-by: Cathy Avery
Applied with Jake's ack to pci/host-hv for v4.8, thanks!
For some reason, Jake's ack appears in patchwork and in my personal
email, but I don't see it on the mailing list. Maybe something in
http://vger.kernel.org/majordomo-info.html#taboo is relevant.
> ---
> drivers/pci/host/pci-hyperv.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 7e9b2de..449d053 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -732,16 +732,18 @@ static void hv_msi_free(struct irq_domain *domain,
> struct msi_domain_info *info,
>
> pdev = msi_desc_to_pci_dev(msi);
> hbus = info->data;
> - hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> - if (!hpdev)
> + int_desc = irq_data_get_irq_chip_data(irq_data);
> + if (!int_desc)
> return;
>
> - int_desc = irq_data_get_irq_chip_data(irq_data);
> - if (int_desc) {
> - irq_data->chip_data = NULL;
> - hv_int_desc_free(hpdev, int_desc);
> + irq_data->chip_data = NULL;
> + hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
> + if (!hpdev) {
> + kfree(int_desc);
> + return;
> }
>
> + hv_int_desc_free(hpdev, int_desc);
> put_pcichild(hpdev, hv_pcidev_ref_by_slot);
> }
>
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next] hv_netvsc: Fix VF register on bonding devices
From: Haiyang Zhang Date: Fri, 22 Jul 2016 18:14:50 -0700 > From: Haiyang Zhang > > Added a condition to avoid bonding devices with same MAC registering > as VF. > > Signed-off-by: Haiyang Zhang > Reviewed-by: K. Y. Srinivasan Applied, thanks. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: declare private functions static
Private functions in ks_hostif.c can be declared static.
Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
extra-repository")
Signed-off-by: Nicholas Mc Guire
---
Found by sparse
The build of ks_hostif.o generated the following sparse warnings:
CHECK drivers/staging/ks7010/ks_hostif.c
drivers/staging/ks7010/ks_hostif.c:72:6: warning: symbol
'ks_wlan_hw_wakeup_task' was not declared. Should it be static?
drivers/staging/ks7010/ks_hostif.c:1512:6: warning: symbol
'hostif_infrastructure_set2_request' was not declared. Should it be static?
As both functions reported are private to this file it seems reasonable
to declare them static.
Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m
Patch is against 4.7-rc7 (localversion-next -next-20160725)
drivers/staging/ks7010/ks_hostif.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index a8822fe..71b6ba2 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -69,7 +69,7 @@ inline u32 get_DWORD(struct ks_wlan_private *priv)
return data;
}
-void ks_wlan_hw_wakeup_task(struct work_struct *work)
+static void ks_wlan_hw_wakeup_task(struct work_struct *work)
{
struct ks_wlan_private *priv =
container_of(work, struct ks_wlan_private, ks_wlan_wakeup_task);
@@ -1505,7 +1505,7 @@ void hostif_infrastructure_set_request(struct
ks_wlan_private *priv)
ks_wlan_hw_tx(priv, pp, hif_align_size(sizeof(*pp)), NULL, NULL, NULL);
}
-void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
+static void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
{
struct hostif_infrastructure_set2_request_t *pp;
uint16_t capability;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: fix wait_for_completion_interruptible_timeout return handling
wait_for_completion_interruptible_timeout return 0 on timeout and
-ERESTARTSYS if interrupted. The check for
!wait_for_completion_interruptible_timeout() would report an interrupt
as timeout. Further, while HZ/50 will work most of the time it could
fail for HZ < 50, so this is switched to msecs_to_jiffies(20).
Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
extra-repository")
Signed-off-by: Nicholas Mc Guire
---
API non-compliance was located by coccinelle
Note: build showed some sparse warnings
CHECK drivers/staging/ks7010/ks_hostif.c
drivers/staging/ks7010/ks_hostif.c:72:6: warning: symbol
'ks_wlan_hw_wakeup_task' was not declared. Should it be static?
drivers/staging/ks7010/ks_hostif.c:1512:6: warning: symbol
'hostif_infrastructure_set2_request' was not declared. Should it be static?
Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m
Patch is against 4.7-rc7 (localversion-next -next-20160725)
drivers/staging/ks7010/ks_hostif.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index a8822fe..4d32c98 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -74,11 +74,15 @@ void ks_wlan_hw_wakeup_task(struct work_struct *work)
struct ks_wlan_private *priv =
container_of(work, struct ks_wlan_private, ks_wlan_wakeup_task);
int ps_status = atomic_read(&priv->psstatus.status);
+ long time_left;
if (ps_status == PS_SNOOZE) {
ks_wlan_hw_wakeup_request(priv);
- if
(!wait_for_completion_interruptible_timeout(&priv->psstatus.wakeup_wait, HZ /
50)) { /* 20ms timeout */
- DPRINTK(1, "wake up timeout !!!\n");
+ time_left = wait_for_completion_interruptible_timeout(
+ &priv->psstatus.wakeup_wait,
+ msecs_to_jiffies(20));
+ if (time_left <= 0) {
+ DPRINTK(1, "wake up timeout or interrupted !!!\n");
schedule_work(&priv->ks_wlan_wakeup_task);
return;
}
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8188eu: Delete an unnecessary check before the function call "vfree"
From: Markus Elfring
Date: Mon, 25 Jul 2016 22:20:24 +0200
The vfree() function performs also input parameter validation.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/staging/rtl8188eu/core/rtw_xmit.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_xmit.c
b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index e0a5567..39ffb46 100644
--- a/drivers/staging/rtl8188eu/core/rtw_xmit.c
+++ b/drivers/staging/rtl8188eu/core/rtw_xmit.c
@@ -252,10 +252,7 @@ void _rtw_free_xmit_priv(struct xmit_priv *pxmitpriv)
pxmitbuf++;
}
- if (pxmitpriv->pallocated_xmit_extbuf) {
- vfree(pxmitpriv->pallocated_xmit_extbuf);
- }
-
+ vfree(pxmitpriv->pallocated_xmit_extbuf);
rtw_free_hwxmits(padapter);
mutex_destroy(&pxmitpriv->ack_tx_mutex);
--
2.9.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: fix wait_for_completion_interruptible_timeout return handling
On Mon, Jul 25, 2016 at 09:21:50PM +0200, Nicholas Mc Guire wrote: > wait_for_completion_interruptible_timeout return 0 on timeout and > -ERESTARTSYS if interrupted. The check for > !wait_for_completion_interruptible_timeout() would report an interrupt > as timeout. Further, while HZ/50 will work most of the time it could Wouldn't it interpret -ERESTARTSYS as *no timeout*? Anyway, the plain !0 comparison for me clearly shows that 'interruptible' was more copy&pasted then really planned or supported. If it was, it would need to cancel something. Also, 20ms is pretty hard to cancel for a user ;) Given all that and the troubles we had with 'interruptible' in the I2C subsystem, I'd much vote for dropping interruptible here. > fail for HZ < 50, so this is switched to msecs_to_jiffies(20). Rest looks good, thanks! signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: ks7010: Rename jump labels
Hello Markus, On lun., 2016-07-25 at 18:19 +0200, SF Markus Elfring wrote: > >> Would you like to support the renaming of a label like "error_out1" > >> (in the function "ks7010_upload_firmware" for example)? > > > > They should be renamed too. Anything using numbers instead of explicit > > Interesting … > > > > Anything using numbers instead of explicit labels should be updated. > > Would you dare to search for corresponding update candidates explicitly > by special semantic patch scripts? No. You started it all, and I do not have more time to devote to it. I do not find it all particularly interesting, to be honest. I have a lot of other things to work on, of much greater interest (to me.) > > I included the reasons in the patch I just sent, > > hopefully the documentation is clearer now. > > I am curious on how feedback will evolve for your suggestion > "CodingStyle: Clarify and complete chapter 7". > https://lkml.org/lkml/2016/7/25/207 > > How do you think about to show a shorter label like "free_bar" > (instead of "err_free_bar") as an example? Up to whoever writes and maintains the code. As most things should be in the absence of a compelling reason to normalize. > >> "Source code review around jump label usage"? > >> https://lkml.org/lkml/2015/12/11/378 > >> http://article.gmane.org/gmane.linux.kernel/2106190 > > > > Personally I see no value in such statistics. > > Do they indicate any code smells eventually? I have no idea what you mean, sorry. > > Either labels are wrong (either wrong indentation or wrong name) > > and should be fixed, or they are correct and you should not touch them. > > Do you find such changes worthwhile (without touching also any surrounding > source code)? You keep asking more and more from me. May I remind you this is your "project" in the first place, not mine? If you have no idea what should be done, or even whether anything should be done, then just move on to something else. I have already expressed all my views on this topic and am not willing to say anything more about it. Thanks, -- Jean Delvare SUSE L3 Support ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: declare private functions static
On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> Private functions in ks_hostif.c can be declared static.
>
> Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> extra-repository")
>
> Signed-off-by: Nicholas Mc Guire
Reviewed-by: Wolfram Sang
drivers/staging/ks7010/ks7010_sdio.c and
drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
like to fix those, too.)
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: dt2811: add parentheses to fix logic on s->subdev_flags
From: Colin Ian King We need to add parentheses around ternary operations because currently the expression SDF_READABLE | (it->options[2] == 1) always evaluates to true, meaning s->subdev_flags is always assigned SDF_DIFF. Putting the parentheses around the ternarary operations results in the intended expression of SDF_REABLE logically or'd with one of SDF_DIFF, SDF_COMMON or SDF_GROUND. Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/dt2811.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/dt2811.c b/drivers/staging/comedi/drivers/dt2811.c index 904f6377..8bbd938 100644 --- a/drivers/staging/comedi/drivers/dt2811.c +++ b/drivers/staging/comedi/drivers/dt2811.c @@ -588,8 +588,8 @@ static int dt2811_attach(struct comedi_device *dev, struct comedi_devconfig *it) s = &dev->subdevices[0]; s->type = COMEDI_SUBD_AI; s->subdev_flags = SDF_READABLE | - (it->options[2] == 1) ? SDF_DIFF : - (it->options[2] == 2) ? SDF_COMMON : SDF_GROUND; + ((it->options[2] == 1) ? SDF_DIFF : + (it->options[2] == 2) ? SDF_COMMON : SDF_GROUND); s->n_chan = (it->options[2] == 1) ? 8 : 16; s->maxdata = 0x0fff; s->range_table = board->is_pgh ? &dt2811_pgh_ai_ranges -- 2.8.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
From: Dexuan Cui
Date: Sat, 23 Jul 2016 01:35:51 +
> +static struct sock *hvsock_create(struct net *net, struct socket *sock,
> + gfp_t priority, unsigned short type)
> +{
> + struct hvsock_sock *hvsk;
> + struct sock *sk;
> +
> + sk = sk_alloc(net, AF_HYPERV, priority, &hvsock_proto, 0);
> + if (!sk)
> + return NULL;
...
> + /* Looks stream-based socket doesn't need this. */
> + sk->sk_backlog_rcv = NULL;
> +
> + sk->sk_state = 0;
> + sock_reset_flag(sk, SOCK_DONE);
All of these are unnecessary initializations, since sk_alloc() zeroes
out the 'sk' object for you.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: David Miller [mailto:da...@davemloft.net]
>
> From: Dexuan Cui
> Date: Sat, 23 Jul 2016 01:35:51 +
>
> > +static struct sock *hvsock_create(struct net *net, struct socket *sock,
> > + gfp_t priority, unsigned short type)
> > +{
> > + struct hvsock_sock *hvsk;
> > + struct sock *sk;
> > +
> > + sk = sk_alloc(net, AF_HYPERV, priority, &hvsock_proto, 0);
> > + if (!sk)
> > + return NULL;
> ...
> > + /* Looks stream-based socket doesn't need this. */
> > + sk->sk_backlog_rcv = NULL;
> > +
> > + sk->sk_state = 0;
> > + sock_reset_flag(sk, SOCK_DONE);
>
> All of these are unnecessary initializations, since sk_alloc() zeroes
> out the 'sk' object for you.
Hi David,
Thanks for the comment! I'll remove the 3 lines.
May I know if you have more comments?
BTW, during the past month, at least 7 other people also reviewed
the patch and gave me quite a few good comments, which have
been addressed. Though only one of them gave the Reviewed-by
line for now, I guess I would get more if I ping them to have a look
at the latest version of the patch, i.e., v19 -- I'm going to post it
with the aforementioned 3 lines removed and if you've more
comments, I'm ready to address them too. :-)
Thanks,
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
From: Dexuan Cui Date: Tue, 26 Jul 2016 03:09:16 + > BTW, during the past month, at least 7 other people also reviewed > the patch and gave me quite a few good comments, which have > been addressed. Correction: Several people gave coding style and simple corrections to your patch. Very few gave any review of the _SUBSTANCE_ of your changes. And the one of the few who did, and suggested you build your facilities using the existing S390 hypervisor socket infrastructure, you brushed off _IMMEDIATELY_. That drives me crazy. The one person who gave you real feedback you basically didn't consider seriously at all. I know why you don't want to consider alternative implementations, and it's because you guys have so much invested in what you've implemented already. But that's tough and not our problem. And until this changes, yes, this submission will be stuck in the mud and continue slogging on like this. Sorry. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v18 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> From: David Miller [mailto:da...@davemloft.net] > ... > From: Dexuan Cui > Date: Tue, 26 Jul 2016 03:09:16 + > > > BTW, during the past month, at least 7 other people also reviewed > > the patch and gave me quite a few good comments, which have > > been addressed. > > Correction: Several people gave coding style and simple corrections > to your patch. > > Very few gave any review of the _SUBSTANCE_ of your changes. > > And the one of the few who did, and suggested you build your > facilities using the existing S390 hypervisor socket infrastructure, > you brushed off _IMMEDIATELY_. > > That drives me crazy. The one person who gave you real feedback > you basically didn't consider seriously at all. Hi David, I'm very sorry -- I guess I must have missed something here -- I don't remember somebody replied with S390 hypervisor socket infrastructure... I'm re-reading all the replies, trying to locate the reply and I'll find out why I didn't take it seriously. Sorry in advance. > I know why you don't want to consider alternative implementations, > and it's because you guys have so much invested in what you've > implemented already. This is not true. I'm absolutely open to any possibility to have an alternative better implementation. Please allow me to find the "S390 hypervisor socket infrastructure" reply first and I'll report back ASAP. > But that's tough and not our problem. > > And until this changes, yes, this submission will be stuck in the > mud and continue slogging on like this. I definitely agree and understand. Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/15] staging/lustre: Add spaces preferred around that '{+,-,*,/,|,<<
On Sat, 2016-07-23 at 14:01 -0400, Oleg Drokin wrote: > On Jul 23, 2016, at 1:31 PM, Joe Perches wrote: [] > > And lustre seems to use types with unnecessary __ prefixes. > Need to see if this file is included in userspace where the __ is needed. Maybe it'd be good to change the top level makefile to add -I and -I and then move the various files appropriately. And all the odd depth #include "/include/" could then be rationalized without regard to source file directory layout. $ grep -roh 'include ".*include.*"' drivers/staging/lustre/|sort|uniq -c|sort -rn 82 include "../include/obd_class.h" 82 include "../../include/linux/libcfs/libcfs.h" 66 include "../include/obd_support.h" 35 include "../include/obd.h" 32 include "../include/lustre_net.h" 32 include "../include/lustre_dlm.h" 27 include "../include/lprocfs_status.h" 26 include "../include/lustre_lite.h" 22 include "../../include/linux/lnet/lib-lnet.h" 21 include "../include/lustre_fid.h" 21 include "../include/cl_object.h" 20 include "../include/lustre/lustre_idl.h" 17 include "../include/lustre_lib.h" 13 include "../../../include/linux/libcfs/libcfs.h" 12 include "../include/lustre_log.h" 9 include "../include/obd_cksum.h" 9 include "../include/lustre_sec.h" 9 include "../include/lustre_req_layout.h" 9 include "../include/lustre_ha.h" 8 include "../include/lustre_ver.h" 8 include "../include/lustre_import.h" 8 include "../../include/linux/lnet/lnetst.h" 7 include "../include/lustre_param.h" 7 include "../include/lustre_mdc.h" 7 include "../../include/linux/lnet/lnet.h" 6 include "../include/lustre_disk.h" 6 include "../include/lustre_debug.h" 5 include "../include/lustre_kernelcomm.h" 5 include "../../include/linux/lnet/lib-types.h" 4 include "../include/lustre/lustre_user.h" 4 include "../include/lustre_intent.h" 4 include "../include/lustre_fld.h" 4 include "../include/lustre_export.h" 4 include "../include/linux/lustre_compat25.h" 4 include "../../include/linux/lnet/lib-dlc.h" 3 include "../include/lustre/ll_fiemap.h" 3 include "../include/lustre_acl.h" 3 include "../../../include/linux/lnet/lnet.h" 3 include "../../../include/linux/lnet/lib-lnet.h" 2 include "../../include/obd_support.h" 2 include "../../include/obd_class.h" 2 include "../include/lustre_mds.h" 2 include "../include/lustre_eacl.h" 2 include "../include/lu_ref.h" 2 include "../include/lu_object.h" 2 include "../../include/lprocfs_status.h" 2 include "../../include/linux/lnet/types.h" 2 include "../../../include/linux/lnet/socklnd.h" 2 include "../../include/linux/lnet/lnetctl.h" 2 include "../../include/linux/libcfs/libcfs_hash.h" 2 include "../../include/linux/libcfs/libcfs_crypto.h" 1 include "../../include/lustre_ver.h" 1 include "../../include/lustre/lustre_idl.h" 1 include "../include/lustre/lustre_errno.h" 1 include "../include/lustre_handles.h" 1 include "../../include/linux/lustre_compat25.h" 1 include "../../../include/linux/lnet/types.h" 1 include "../../include/linux/lnet/nidstr.h" 1 include "../../../include/linux/lnet/lnetctl.h" 1 include "../../include/linux/lnet/api.h" 1 include "../../../include/linux/libcfs/libcfs_crypto.h" 1 include "../include/interval_tree.h" ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()
>> -if (strncasecmp(buff, "RSSI", length) == 0) {
>> +if (strncasecmp(buff, "RSSI", 0) == 0) {
>> +s8 rssi;
>> +
>
> Um, please think a second about if it makes any sense at all to compare
> zero chars of two strings.
Under which circumstances should the variable "length" contain an other
value than zero?
How can this open issue be fixed better?
Regards,
Markus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: declare private functions static
On Tue, Jul 26, 2016 at 06:48:00AM +, Nicholas Mc Guire wrote:
> On Mon, Jul 25, 2016 at 11:04:18PM +0200, Wolfram Sang wrote:
> > On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> > > Private functions in ks_hostif.c can be declared static.
> > >
> > > Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> > > extra-repository")
> > >
> > > Signed-off-by: Nicholas Mc Guire
> >
> > Reviewed-by: Wolfram Sang
> >
> > drivers/staging/ks7010/ks7010_sdio.c and
> > drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
> > like to fix those, too.)
> >
> the cases found regarding completion were:
> ./drivers/staging/ks7010/ks_hostif.c:80 treating signal case as success
> ./drivers/staging/ks7010/ks_wlan_net.c:109 treating signal case as success
> ./drivers/staging/ks7010/ks7010_sdio.c:901 treating signal case as success
> ./drivers/staging/ks7010/ks7010_sdio.c:929 treating signal case as success
> ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:383 treating signal
> case as success
> ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:247 treating signal
> case as success
>
> will be going through all of them in the next days.
Awesome, thanks!
I meant the "should it be static?" sparse warnings here, though :)
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: fix wait_for_completion_interruptible_timeout return handling
On Mon, Jul 25, 2016 at 10:54:03PM +0200, Wolfram Sang wrote: > On Mon, Jul 25, 2016 at 09:21:50PM +0200, Nicholas Mc Guire wrote: > > wait_for_completion_interruptible_timeout return 0 on timeout and > > -ERESTARTSYS if interrupted. The check for > > !wait_for_completion_interruptible_timeout() would report an interrupt > > as timeout. Further, while HZ/50 will work most of the time it could > > Wouldn't it interpret -ERESTARTSYS as *no timeout*? > yup - actually the current code just treats the -ERESTARTSYS case as success. > Anyway, the plain !0 comparison for me clearly shows that > 'interruptible' was more copy&pasted then really planned or supported. > If it was, it would need to cancel something. Also, 20ms is pretty hard > to cancel for a user ;) Given all that and the troubles we had with > 'interruptible' in the I2C subsystem, I'd much vote for dropping > interruptible here. > > > fail for HZ < 50, so this is switched to msecs_to_jiffies(20). > > Rest looks good, thanks! > thx! hofrat ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: declare private functions static
On Tue, Jul 26, 2016 at 08:51:14AM +0200, Wolfram Sang wrote:
> On Tue, Jul 26, 2016 at 06:48:00AM +, Nicholas Mc Guire wrote:
> > On Mon, Jul 25, 2016 at 11:04:18PM +0200, Wolfram Sang wrote:
> > > On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> > > > Private functions in ks_hostif.c can be declared static.
> > > >
> > > > Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> > > > extra-repository")
> > > >
> > > > Signed-off-by: Nicholas Mc Guire
> > >
> > > Reviewed-by: Wolfram Sang
> > >
> > > drivers/staging/ks7010/ks7010_sdio.c and
> > > drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
> > > like to fix those, too.)
> > >
> > the cases found regarding completion were:
> > ./drivers/staging/ks7010/ks_hostif.c:80 treating signal case as success
> > ./drivers/staging/ks7010/ks_wlan_net.c:109 treating signal case as success
> > ./drivers/staging/ks7010/ks7010_sdio.c:901 treating signal case as success
> > ./drivers/staging/ks7010/ks7010_sdio.c:929 treating signal case as success
> > ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:383 treating signal
> > case as success
> > ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:247 treating signal
> > case as success
> >
> > will be going through all of them in the next days.
>
> Awesome, thanks!
>
> I meant the "should it be static?" sparse warnings here, though :)
>
well I do run sparse on all the cleanups and if that triggers
and it is sufficiently clear from context, patches will follow.
thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: declare private functions static
On Mon, Jul 25, 2016 at 11:04:18PM +0200, Wolfram Sang wrote:
> On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> > Private functions in ks_hostif.c can be declared static.
> >
> > Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote
> > extra-repository")
> >
> > Signed-off-by: Nicholas Mc Guire
>
> Reviewed-by: Wolfram Sang
>
> drivers/staging/ks7010/ks7010_sdio.c and
> drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
> like to fix those, too.)
>
the cases found regarding completion were:
./drivers/staging/ks7010/ks_hostif.c:80 treating signal case as success
./drivers/staging/ks7010/ks_wlan_net.c:109 treating signal case as success
./drivers/staging/ks7010/ks7010_sdio.c:901 treating signal case as success
./drivers/staging/ks7010/ks7010_sdio.c:929 treating signal case as success
./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:383 treating signal case
as success
./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:247 treating signal case
as success
will be going through all of them in the next days.
thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
