Re: [PATCH net-next] sun: Add SPDX license tags to Sun network drivers
Hi Shannon, On Wed, Feb 7, 2018 at 6:34 AM, Shannon Nelson <shannon.nel...@oracle.com> wrote: > Add the appropriate SPDX license tags to the Sun network drivers > as outlined in Documentation/process/license-rules.rst. > > Signed-off-by: Shannon Nelson <shannon.nel...@oracle.com> > --- > drivers/net/ethernet/sun/Kconfig | 1 + > drivers/net/ethernet/sun/cassini.c| 1 + > drivers/net/ethernet/sun/cassini.h| 1 + > drivers/net/ethernet/sun/ldmvsw.c | 1 + > drivers/net/ethernet/sun/niu.c| 1 + > drivers/net/ethernet/sun/sunbmac.c| 1 + > drivers/net/ethernet/sun/sungem.c | 1 + > drivers/net/ethernet/sun/sunhme.c | 1 + > drivers/net/ethernet/sun/sunqe.c | 1 + > drivers/net/ethernet/sun/sunvnet.c| 1 + > drivers/net/ethernet/sun/sunvnet_common.c | 1 + > 11 files changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/sun/Kconfig > b/drivers/net/ethernet/sun/Kconfig > index b2caf51..7b982e0 100644 > --- a/drivers/net/ethernet/sun/Kconfig > +++ b/drivers/net/ethernet/sun/Kconfig > @@ -1,3 +1,4 @@ > +# SPDX-License-Identifier: GPL-2.0 > # > # Sun network device configuration > # I'm not sure that Kconfig files count as source, right? > diff --git a/drivers/net/ethernet/sun/cassini.c > b/drivers/net/ethernet/sun/cassini.c > index 113bd57..9020b08 100644 > --- a/drivers/net/ethernet/sun/cassini.c > +++ b/drivers/net/ethernet/sun/cassini.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0 > /* cassini.c: Sun Microsystems Cassini(+) ethernet driver. > * > * Copyright (C) 2004 Sun Microsystems Inc. I understand that this is the specified way to do this, but it's exceptionally ugly. Also, shouldn't the SPDX line _replace_ the usual "this program is free software" license paragraphs? My understanding is that the SPDX line is functionally equivalent to having the terms spelled out. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [linux-sunxi] [PATCH 1/3] net: stmmac: dwmac-sun8i: drop V3s compatible and add V3 one
Hi Icenowy, On Sat, Feb 3, 2018 at 5:04 AM, Icenowy Zheng <icen...@aosc.io> wrote: > The V3s is just a differently packaged version of the V3 chip, which has > a MAC with the same capability with H3. The V3s just doesn't wire out > the external MII/RMII/RGMII bus. (V3 wired out it). > > Drop the compatible string of V3s in the dwmac-sun8i driver, and add a > V3 compatible string, which has all capabilities. Aren't compatible strings technically API, so don't we need to support those that are out in the wild "forever"? Therefore shouldn't we leave the v3s variant around for compatibility with existing device trees? Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 5/5] wlcore: Use common error handling code in wl1271_op_suspend()
Hi Markus,
On Mon, Oct 30, 2017 at 7:16 AM, SF Markus Elfring
<elfr...@users.sourceforge.net> wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Sun, 29 Oct 2017 20:36:39 +0100
>
> Add a jump target so that a specific error message is stored only once
> at the end of this function implementation.
> Replace two calls of the macro "wl1271_warning" by goto statements.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
This patch is bogus, moving error messages around like this is just bizarre.
These are both reporting different failures, so the error messages
should be different.
> ---
> drivers/net/wireless/ti/wlcore/main.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/main.c
> b/drivers/net/wireless/ti/wlcore/main.c
> index 12a9d6509382..a110f61110d5 100644
> --- a/drivers/net/wireless/ti/wlcore/main.c
> +++ b/drivers/net/wireless/ti/wlcore/main.c
> @@ -1732,8 +1732,7 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
> ret = wl1271_configure_suspend(wl, wlvif, wow);
> if (ret < 0) {
> mutex_unlock(>mutex);
> - wl1271_warning("couldn't prepare device to suspend");
"couldn't configure device for suspend"?
> - return ret;
> + goto report_preparation_failure;
> }
> }
>
> @@ -1752,10 +1751,8 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
> wl1271_ps_elp_sleep(wl);
> mutex_unlock(>mutex);
>
> - if (ret < 0) {
> - wl1271_warning("couldn't prepare device to suspend");
"couldn't enable power saving"?
> - return ret;
> - }
> + if (ret < 0)
> + goto report_preparation_failure;
>
> /* flush any remaining work */
> wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 4/5] wlcore: Use common error handling code in wlcore_set_beacon_template()
Hi Markus, On Mon, Oct 30, 2017 at 7:14 AM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Sun, 29 Oct 2017 20:16:22 +0100 > > Adjust jump targets so that a bit of exception handling can be better > reused at the end of this function. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> However, > --- > drivers/net/wireless/ti/wlcore/main.c | 18 +++--- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/wireless/ti/wlcore/main.c > b/drivers/net/wireless/ti/wlcore/main.c > index 0365b5e40a8d..12a9d6509382 100644 > --- a/drivers/net/wireless/ti/wlcore/main.c > +++ b/drivers/net/wireless/ti/wlcore/main.c > @@ -4081,9 +4078,8 @@ static int wlcore_set_beacon_template(struct wl1271 *wl, > beacon->data, > beacon->len, 0, > min_rate); > -end_bcn: > +free_skb: Why rename the label? Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 3/5] wlcore: Return directly after a failed ieee80211_beacon_get() in wlcore_set_beacon_template()
On Mon, Oct 30, 2017 at 7:13 AM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Sun, 29 Oct 2017 20:00:41 +0100 > > Return directly after a call of the function "ieee80211_beacon_get" > failed at the beginning. > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 1/5] wlcore: Use common error handling code in wlcore_nvs_cb()
Hi Markus, On Mon, Oct 30, 2017 at 7:11 AM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Sun, 29 Oct 2017 18:38:04 +0100 > > Add a jump target so that a bit of exception handling can be better reused > at the end of this function. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> > --- > drivers/net/wireless/ti/wlcore/main.c | 11 +++ > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/ti/wlcore/main.c > b/drivers/net/wireless/ti/wlcore/main.c > index c346c021b999..48380d48ef09 100644 > --- a/drivers/net/wireless/ti/wlcore/main.c > +++ b/drivers/net/wireless/ti/wlcore/main.c > @@ -6551,6 +6549,11 @@ static void wlcore_nvs_cb(const struct firmware *fw, > void *context) > out: > release_firmware(fw); > complete_all(>nvs_loading_complete); > + return; > + > +power_off: Name this "out_power_off" to match the other labels. > + wl1271_power_off(wl); > + goto out_free_nvs; Why not put this in front of the out_free_nvs label? It looks weird here. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/5] wlcore: Delete an unnecessary check statement in wlcore_set_beacon_template()
On Mon, Oct 30, 2017 at 7:12 AM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Sun, 29 Oct 2017 19:45:07 +0100 > > A goto statement jumped to a target which followed a condition check > immediately without the specification of useful actions between. > Thus remove such unnecessary source code at the end of this function. > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> Looks good to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 28/44] sparc: remove arch specific dma_supported implementations
Hi Christoph,
On Thu, Jun 8, 2017 at 11:25 PM, Christoph Hellwig <h...@lst.de> wrote:
> Usually dma_supported decisions are done by the dma_map_ops instance.
> Switch sparc to that model by providing a ->dma_supported instance for
> sbus that always returns false, and implementations tailored to the sun4u
> and sun4v cases for sparc64, and leave it unimplemented for PCI on
> sparc32, which means always supported.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> ---
> arch/sparc/include/asm/dma-mapping.h | 3 ---
> arch/sparc/kernel/iommu.c| 40
> +++-
> arch/sparc/kernel/ioport.c | 22 ++--
> arch/sparc/kernel/pci_sun4v.c| 17 +++
> 4 files changed, 39 insertions(+), 43 deletions(-)
>
> diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
> index dd081d557609..12894f259bea 100644
> --- a/arch/sparc/kernel/ioport.c
> +++ b/arch/sparc/kernel/ioport.c
> @@ -401,6 +401,11 @@ static void sbus_sync_sg_for_device(struct device *dev,
> struct scatterlist *sg,
> BUG();
> }
>
> +static int sbus_dma_supported(struct device *dev, u64 mask)
> +{
> + return 0;
> +}
> +
I'm guessing there's a few places that have DMA ops but DMA isn't
actually supported. Why not have a common method for this, maybe
"dma_not_supported"?
> static const struct dma_map_ops sbus_dma_ops = {
> .alloc = sbus_alloc_coherent,
> .free = sbus_free_coherent,
> @@ -410,6 +415,7 @@ static const struct dma_map_ops sbus_dma_ops = {
> .unmap_sg = sbus_unmap_sg,
> .sync_sg_for_cpu= sbus_sync_sg_for_cpu,
> .sync_sg_for_device = sbus_sync_sg_for_device,
> + .dma_supported = sbus_dma_supported,
> };
>
> static int __init sparc_register_ioport(void)
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mwifiex: fix null pointer deference when adapter is null
Hi All, On Thu, Sep 15, 2016 at 11:42 PM, Colin King <colin.k...@canonical.com> wrote: > From: Colin Ian King <colin.k...@canonical.com> > > If adapter is null the error exit path in mwifiex_shutdown_sw is > to down the semaphore sem and print some debug via mwifiex_dbg. > However, passing a NULL adapter to mwifiex_dbg causes a null > pointer deference when accessing adapter->dev. This fix checks > for a null adapter at the start of the function and to exit > without the need to up the semaphore and we also skip the debug > to avoid the null pointer dereference. > > Signed-off-by: Colin Ian King <colin.k...@canonical.com> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/2] mwifiex: simplify length computation for some memset
Hi All, On Mon, Aug 8, 2016 at 5:39 PM, Christophe JAILLET <christophe.jail...@wanadoo.fr> wrote: > This patch should be a no-op. It just simplifies code by using the name of > a variable instead of its type when calling 'sizeof'. > > Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] ath10k: Spelling and miscellaneous neatening
Hi All, On Tue, Aug 30, 2016 at 3:05 AM, Joe Perches <j...@perches.com> wrote: > Correct some trivial comment typos. > Remove unnecessary parentheses in a long line. > Convert a return; before the end of a void function definition to just ; > > Signed-off-by: Joe Perches <j...@perches.com> This all looks correct to me. I wish you'd put the code changes in a separate patch, however it's all noted in the commit log, so... Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] rtl8xxxu: fix spelling mistake "firmare" -> "firmware"
Hi All, On Sun, Sep 4, 2016 at 2:43 AM, Colin King <colin.k...@canonical.com> wrote: > From: Colin Ian King <colin.k...@canonical.com> > > Trivial fix to spelling mistakes in dev_dbg message. > > Signed-off-by: Colin Ian King <colin.k...@canonical.com> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] ath10k: fix spelling mistake "montior" -> "monitor"
Hi All, On Sat, Aug 27, 2016 at 4:08 AM, Colin King <colin.k...@canonical.com> wrote: > From: Colin Ian King <colin.k...@canonical.com> > > Trivial fix to spelling mistake in ath10k_warn message. > > Signed-off-by: Colin Ian King <colin.k...@canonical.com> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] zd1211rw: fix spelling mistake "firmeware" -> "firmware"
Hi All, On Tue, Aug 23, 2016 at 4:35 AM, Colin King <colin.k...@canonical.com> wrote: > From: Colin Ian King <colin.k...@canonical.com> > > Trivial fix to spelling mistake in dev_err message. > > Signed-off-by: Colin Ian King <colin.k...@canonical.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
Hi Marcus,
On Sun, Aug 21, 2016 at 2:46 AM, SF Markus Elfring
<elfr...@users.sourceforge.net> wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> ---
> drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> index 4e271f9..5942917 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> @@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t
> *local, struct iw_point *p)
> }
>
> if (ret == 1 || !ap_ioctl) {
> - if (copy_to_user(p->pointer, param, p->length)) {
> + if (copy_to_user(p->pointer, param, p->length))
> ret = -EFAULT;
> - goto out;
> - } else if (ap_ioctl)
> + else if (ap_ioctl)
> ret = 0;
> }
>
> - out:
Does this change make any difference to the compiled code?
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mwifiex: fix unconditional error return in .add_virtual_intf callback
Hi All,
On Sat, Jul 2, 2016 at 5:39 AM, Javier Martinez Canillas
<jav...@osg.samsung.com> wrote:
> The commit 7311ea850079 ("mwifiex: fix AP start problem for newly added
> interface") attempted to fix an issue when a new AP interface is added.
>
> But the patch didn't check the return value of the functions doing the
> firmware calls and returned an error even if the functions didn't fail.
>
> This prevents the network device to be registered properly, so fix it.
>
> Fixes: commit 7311ea850079 ("mwifiex: fix AP start problem for newly added
> interface")
> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Looks correct to me as Dan Carpenter submitted the same fix.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 0/4] Mesh mpm fixes and enhancements
Hi Yaniv, On Tue, Jun 28, 2016 at 9:13 PM, Yaniv Machani <yani...@ti.com> wrote: > This patch set is addressing some issues found in the current 802.11s > implementation, > specifically when using hostap mpm. > It's aligning the beacon format and handling some corner cases. > > Maital Hahn (2): > mac80211: mesh: flush stations before beacons are stopped > mac80211/cfg: mesh: fix healing time when a mesh peer is disconnecting > > Meirav Kama (2): > mac80211: mesh: fixed HT ies in beacon template > mac80211: sta_info: max_peers reached falsely Patches that you send must be signed off by you, not ack'd by you. I.e. From: Random Developer <random.develo...@company.com> . Signed-off-by: Random Developer <random.develo...@company.com> Signed-off-by: Patch Sender <patch.sen...@company.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] [linux-next] rtlwifi: Fix typo in printk
Hi All, On Wed, Jun 29, 2016 at 1:37 PM, Masanari Iida <standby2...@gmail.com> wrote: > This patch fix spelling typos found in drivers/net/wireless/realtek. > > Signed-off-by: Masanari Iida <standby2...@gmail.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing
Hi Javier, On Wed, Jun 1, 2016 at 11:51 PM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > Hello Julian, > > Thanks a lot for your feedback and reviews. > > On 06/01/2016 12:20 AM, Julian Calaby wrote: >> Hi All, >> >> On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas >> <jav...@osg.samsung.com> wrote: >>> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT >>> binding document say that the "interrupts" property in the child node is >>> optional. So the property being missed shouldn't be treated as an error. >> >> Have you checked whether it is truly optional? I.e. nothing else >> breaks if this property isn't set? >> > > That's what the DT binding says and the IRQ is only used as a wakeup source > during system suspend, it is not used during runtime. And that is why the > mwifiex_sdio_probe_of() function does not fail if the IRQ is missing. Awesome, that's what I wanted to know. > Now, I just got to that conclusion by reading the binding docs, the message > in the commits that introduced this and the driver code. Xinming Hu should > comment on how critical this feature is for systems that needs to be wakeup. Xinming, could you review this also? > In any case I think that the code should be consistent with what the binding > doc says and also the function does (i.e: dev_err only if returns an error). > >>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> >> >> Other than that, this looks sensible to me. >> >> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> >> > > Best regards, Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 8/8] mwifiex: use better message and error code when OF node doesn't match
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT > binding document lists the possible compatible strings that a SDIO child > node can have, so the driver checks if the defined in the node matches. > > But the error message when that's not the case is misleading, so change > for one that makes clear what the error really is. Also, returning a -1 > as errno code is not correct since that's -EPERM. A -EINVAL seems to be > a more appropriate one. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT > binding document say that the "interrupts" property in the child node is > optional. So the property being missed shouldn't be treated as an error. Have you checked whether it is truly optional? I.e. nothing else breaks if this property isn't set? > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> Other than that, this looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 6/8] mwifiex: check if mwifiex_sdio_probe_of() fails and return error
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > The function can fail so the returned value should be checked > and the error propagated to the caller in case of a failure. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > Instead of duplicating part of the cleanups needed in case of an error > in .probe callback, have a single error path and use goto labels as is > common practice in the kernel. > > This also has the nice side effect that the cleanup operations are made > in the inverse order of their counterparts, which was not the case for > the mwifiex_add_card() error path. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++-- > 1 file changed, 11 insertions(+), 6 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 3/8] mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > There's only a check if mwifiex_add_card() returned a nonzero value, but > the actual error code is neither stored nor propagated to the caller. So > instead of always returning -1 (which is -EPERM and not a suitable errno > code in this case), propagate the value returned by mwifiex_add_card(). > > Patch also removes the assignment of sdio_disable_func() returned value > since it was overwritten anyways and what matters is to know the error > value returned by the first function that failed. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 5/8] mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > It's better to have the device name prefixed in the error message. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/8] mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()
Hi All, On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas <jav...@osg.samsung.com> wrote: > If the sdio_enable_func() function fails on .probe, the -EIO errno code > is always returned but that could make more difficult to debug and find > the cause of why the function actually failed. > > Since the driver/device core prints the value returned by .probe in its > error message propagate what was returned by sdio_enable_func() at fail. > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com> This looks sensible to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
Hi All,
On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas
<jav...@osg.samsung.com> wrote:
> SDIO is an auto enumerable bus so the SDIO devices are matched using the
> sdio_device_id table and not using compatible strings from a OF id table.
>
> However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup
> interrupt support") allowed to match nodes defined as child of the SDIO
> host controller in the probe function using a compatible string to setup
> platform specific parameters in the DT.
>
> The problem is that the OF parse function is always called regardless if
> the SDIO dev has an OF node associated or not, and prints an error if it
> is not found. So, on a platform that doesn't have a node for a SDIO dev,
> the following misleading error message will be printed:
>
> [ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available
>
> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
This looks sensible to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] net/wireless: Fix 'multiple blank lines' check
Hi All, On Tue, May 31, 2016 at 3:58 PM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: > This patch fixes the checkpatch.pl check "Please don't use multiple > blank lines" for all files in net/wireless, except nl80211.c which > is covered in a separated patch. > > Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > net/wireless/ap.c | 1 - > net/wireless/chan.c| 3 --- > net/wireless/core.h| 2 -- > net/wireless/ibss.c| 1 - > net/wireless/mlme.c| 2 -- > net/wireless/radiotap.c| 2 -- > net/wireless/rdev-ops.h| 2 -- > net/wireless/reg.c | 3 --- > net/wireless/scan.c| 1 - > net/wireless/sme.c | 1 - > net/wireless/util.c| 2 -- > net/wireless/wext-compat.c | 3 --- > net/wireless/wext-compat.h | 2 -- > net/wireless/wext-core.c | 6 -- > net/wireless/wext-proc.c | 1 - > net/wireless/wext-sme.c| 1 - > 16 files changed, 33 deletions(-) Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 06/10] nl80211: Various checkpatch.pl spacing fixes
Hi All,
On Mon, May 30, 2016 at 12:53 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the following spacing issues reported
> by checkpatch.pl -
> - space preferred around that
> - no space needed after cast.
> - Alignment should match open parenthesis
> - suspect code indent for conditional statements
> - Statements should start on a tabstop
>
> This patch also contains two hunks to fix 'line over 80 characters',
> that are spacing related.
> All other instances of that warning have been ignored.
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
With Kirtika's explanation, this is:
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 103
> ++---
> 1 file changed, 54 insertions(+), 49 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 11cbf0b..ad7cdce 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -39,10 +39,10 @@ static void nl80211_post_doit(const struct genl_ops *ops,
> struct sk_buff *skb,
>
> /* the netlink family */
> static struct genl_family nl80211_fam = {
> - .id = GENL_ID_GENERATE, /* don't bother with a hardcoded ID */
> - .name = NL80211_GENL_NAME, /* have users key off the name
> instead */
> - .hdrsize = 0, /* no private header */
> - .version = 1, /* no particular meaning now */
> + .id = GENL_ID_GENERATE, /* don't bother with a hardcoded ID */
> + .name = NL80211_GENL_NAME, /* have users key off the name instead */
> + .hdrsize = 0, /* no private header */
> + .version = 1, /* no particular meaning now */
> .maxattr = NL80211_ATTR_MAX,
> .netnsok = true,
> .pre_doit = nl80211_pre_doit,
> @@ -213,7 +213,7 @@ cfg80211_get_dev_from_info(struct net *netns, struct
> genl_info *info)
> static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
> [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
> - .len = 20-1 },
> + .len = 20 - 1 },
> [NL80211_ATTR_WIPHY_TXQ_PARAMS] = { .type = NLA_NESTED },
>
> [NL80211_ATTR_WIPHY_FREQ] = { .type = NLA_U32 },
> @@ -231,7 +231,7 @@ static const struct nla_policy
> nl80211_policy[NUM_NL80211_ATTR] = {
>
> [NL80211_ATTR_IFTYPE] = { .type = NLA_U32 },
> [NL80211_ATTR_IFINDEX] = { .type = NLA_U32 },
> - [NL80211_ATTR_IFNAME] = { .type = NLA_NUL_STRING, .len = IFNAMSIZ-1 },
> + [NL80211_ATTR_IFNAME] = { .type = NLA_NUL_STRING, .len = IFNAMSIZ - 1
> },
>
> [NL80211_ATTR_MAC] = { .len = ETH_ALEN },
> [NL80211_ATTR_PREV_BSSID] = { .len = ETH_ALEN },
> @@ -967,7 +967,7 @@ static int nl80211_put_iface_combinations(struct wiphy
> *wiphy,
> int i, j;
>
> nl_combis = nla_nest_start(msg,
> - NL80211_ATTR_INTERFACE_COMBINATIONS);
> + NL80211_ATTR_INTERFACE_COMBINATIONS);
> if (!nl_combis)
> goto nla_put_failure;
>
> @@ -1012,9 +1012,9 @@ static int nl80211_put_iface_combinations(struct wiphy
> *wiphy,
> goto nla_put_failure;
> if (large &&
> (nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_WIDTHS,
> - c->radar_detect_widths) ||
> +c->radar_detect_widths) ||
> nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_REGIONS,
> - c->radar_detect_regions)))
> +c->radar_detect_regions)))
> goto nla_put_failure;
>
> nla_nest_end(msg, nl_combi);
> @@ -1493,7 +1493,7 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
>
> i = 0;
> #define CMD(op, n) \
> -do { \
> + do {\
> if (rdev->ops->op) {\
> i++;\
> if (nla_put_u32(msg, i, NL80211_CMD_ ## n)) \
> @@ -1735,8 +1735,9 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
>
Re: [PATCH v2 06/10] nl80211: Various checkpatch.pl spacing fixes
Hi Kirtika, On Mon, May 30, 2016 at 4:04 PM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: >> Adding the brackets around the & expression doesn't look spacing >> related to me. What's the exact warning this is fixing? > > From the commit message - "This patch also contains two hunks to fix > 'line over 80 characters', > that are spacing related". This is the second hunk, the first being > the comments in the nl80211_fam > definition. Should I resend with these two hunks omitted, or fix my wording? That explains it, I missed that bit. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 10/10] nl80211: Prefer kcalloc over kzalloc
Hi All, On Mon, May 30, 2016 at 12:54 PM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: > This patch fixes the checkpatch.pl warning - > "prefer kcalloc over kzalloc with multiply" > > Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, Julian Calaby > --- > net/wireless/nl80211.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index 46757af..2964406 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -9482,7 +9482,7 @@ static int nl80211_parse_wowlan_nd(struct > cfg80211_registered_device *rdev, > struct nlattr **tb; > int err; > > - tb = kzalloc(NUM_NL80211_ATTR * sizeof(*tb), GFP_KERNEL); > + tb = kcalloc(NUM_NL80211_ATTR, sizeof(*tb), GFP_KERNEL); > if (!tb) > return -ENOMEM; > > -- > 2.8.0.rc3.226.g39d4020 > -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 09/10] nl80211: Fix checkpatch.pl warning
Hi All,
On Mon, May 30, 2016 at 12:54 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the checkpatch.pl warning "foo * bar should be
> foo *bar"
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index c6d870e..46757af 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -12214,7 +12214,7 @@ void nl80211_send_ibss_bssid(struct
> cfg80211_registered_device *rdev,
> }
>
> void cfg80211_notify_new_peer_candidate(struct net_device *dev, const u8
> *addr,
> - const u8* ie, u8 ie_len, gfp_t gfp)
> + const u8 *ie, u8 ie_len, gfp_t gfp)
> {
> struct wireless_dev *wdev = dev->ieee80211_ptr;
> struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
> @@ -13362,7 +13362,7 @@ void cfg80211_tdls_oper_request(struct net_device
> *dev, const u8 *peer,
> }
> EXPORT_SYMBOL(cfg80211_tdls_oper_request);
>
> -static int nl80211_netlink_notify(struct notifier_block * nb,
> +static int nl80211_netlink_notify(struct notifier_block *nb,
> unsigned long state,
> void *_notify)
> {
> --
> 2.8.0.rc3.226.g39d4020
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 08/10] nl80211: Fix spelling
Hi All, On Mon, May 30, 2016 at 12:54 PM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: > Fix 'implementation' spelling, reported by checkpatch.pl > > Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, Julian Calaby > --- > net/wireless/nl80211.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index 6f8e2a7..c6d870e 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -6492,7 +6492,7 @@ nl80211_parse_sched_scan(struct wiphy *wiphy, struct > wireless_dev *wdev, >nla_data(ssid), nla_len(ssid)); > request->match_sets[i].ssid.ssid_len = > nla_len(ssid); > - /* special attribute - old implemenation w/a > */ > + /* special attribute - old implementation w/a > */ > request->match_sets[i].rssi_thold = > default_match_rssi; > rssi = tb[NL80211_SCHED_SCAN_MATCH_ATTR_RSSI]; > -- > 2.8.0.rc3.226.g39d4020 > -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 07/10] nl80211: Avoid multiple assignments on same line
Hi All,
On Mon, May 30, 2016 at 12:53 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the checkpatch.pl warning "multiple assignments
> should be avoided."
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
I'm not sure I agree with checkpatch here, but this looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index ad7cdce..6f8e2a7 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -2603,7 +2603,8 @@ static int nl80211_set_interface(struct sk_buff *skb,
> struct genl_info *info)
>
> memset(, 0, sizeof(params));
>
> - otype = ntype = dev->ieee80211_ptr->iftype;
> + otype = dev->ieee80211_ptr->iftype;
> + ntype = dev->ieee80211_ptr->iftype;
>
> if (info->attrs[NL80211_ATTR_IFTYPE]) {
> ntype = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
> --
> 2.8.0.rc3.226.g39d4020
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 06/10] nl80211: Various checkpatch.pl spacing fixes
Hi All, On Mon, May 30, 2016 at 12:53 PM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: > This patch fixes the following spacing issues reported > by checkpatch.pl - > - space preferred around that > - no space needed after cast. > - Alignment should match open parenthesis > - suspect code indent for conditional statements > - Statements should start on a tabstop > > This patch also contains two hunks to fix 'line over 80 characters', > that are spacing related. > All other instances of that warning have been ignored. > > Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com> Looks right to me except for one minor point: > --- > net/wireless/nl80211.c | 103 > ++--- > 1 file changed, 54 insertions(+), 49 deletions(-) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index 11cbf0b..ad7cdce 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -1735,8 +1735,9 @@ static int nl80211_send_wiphy(struct > cfg80211_registered_device *rdev, >rdev->wiphy.max_num_csa_counters)) > goto nla_put_failure; > > - if (rdev->wiphy.regulatory_flags & > REGULATORY_WIPHY_SELF_MANAGED && > - nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)) > + if ((rdev->wiphy.regulatory_flags & > +REGULATORY_WIPHY_SELF_MANAGED) && > +nla_put_flag(msg, NL80211_ATTR_WIPHY_SELF_MANAGED_REG)) Adding the brackets around the & expression doesn't look spacing related to me. What's the exact warning this is fixing? > goto nla_put_failure; > > if (nla_put(msg, NL80211_ATTR_EXT_FEATURES, Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 05/10] nl80211: Fix checkpatch.pl NULL comparison warning
Hi All,
On Mon, May 30, 2016 at 12:52 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the warning - "comparison to NULL (foo == NULL)
> could be written as (!foo)"
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 945405d..11cbf0b 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -928,7 +928,7 @@ static struct ieee80211_channel
> *nl80211_get_valid_chan(struct wiphy *wiphy,
> {
> struct ieee80211_channel *chan;
>
> - if (tb == NULL)
> + if (!tb)
> return NULL;
> chan = ieee80211_get_channel(wiphy, nla_get_u32(tb));
> if (!chan || chan->flags & IEEE80211_CHAN_DISABLED)
> @@ -8575,7 +8575,7 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff
> *skb,
> if (band < 0 || band >= NUM_NL80211_BANDS)
> return -EINVAL;
> sband = rdev->wiphy.bands[band];
> - if (sband == NULL)
> + if (!sband)
> return -EINVAL;
> err = nla_parse(tb, NL80211_TXRATE_MAX, nla_data(tx_rates),
> nla_len(tx_rates), nl80211_txattr_policy);
> --
> 2.8.0.rc3.226.g39d4020
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 04/10] nl80211: Fix checkpatch.pl warning about braces
Hi All,
On Mon, May 30, 2016 at 12:52 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the following checkpatch,pl warning -
> - braces {} should be used on all arms of this statement.
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 34b8fbe..945405d 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -870,8 +870,9 @@ nl80211_parse_connkeys(struct cfg80211_registered_device
> *rdev,
> result->def = parse.idx;
> if (!parse.def_uni || !parse.def_multi)
> goto error;
> - } else if (parse.defmgmt)
> + } else if (parse.defmgmt) {
> goto error;
> + }
> err = cfg80211_validate_key_settings(rdev, ,
> parse.idx, false, NULL);
> if (err)
> @@ -1401,8 +1402,9 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
> break;
> case 2:
> if (nl80211_put_iftypes(msg, NL80211_ATTR_SUPPORTED_IFTYPES,
> - rdev->wiphy.interface_modes))
> - goto nla_put_failure;
> + rdev->wiphy.interface_modes)) {
> + goto nla_put_failure;
> + }
> state->split_start++;
> if (state->split)
> break;
> @@ -2155,8 +2157,9 @@ static int nl80211_set_wiphy(struct sk_buff *skb,
> struct genl_info *info)
> wdev = NULL;
> netdev = NULL;
> result = 0;
> - } else
> + } else {
> wdev = netdev->ieee80211_ptr;
> + }
>
> /* end workaround code, by now the rdev is available
> * and locked, and wdev may or may not be NULL.
> @@ -3403,8 +3406,9 @@ static int nl80211_start_ap(struct sk_buff *skb, struct
> genl_info *info)
> if (!nl80211_valid_auth_type(rdev, params.auth_type,
> NL80211_CMD_START_AP))
> return -EINVAL;
> - } else
> + } else {
> params.auth_type = NL80211_AUTHTYPE_AUTOMATIC;
> + }
>
> err = nl80211_crypto_settings(rdev, info, ,
> NL80211_MAX_NR_CIPHER_SUITES);
> @@ -3450,8 +3454,9 @@ static int nl80211_start_ap(struct sk_buff *skb, struct
> genl_info *info)
> return err;
> } else if (wdev->preset_chandef.chan) {
> params.chandef = wdev->preset_chandef;
> - } else if (!nl80211_get_ap_channel(rdev, ))
> + } else if (!nl80211_get_ap_channel(rdev, )) {
> return -EINVAL;
> + }
>
> if (!cfg80211_reg_can_beacon_relax(>wiphy, ,
>wdev->iftype))
> @@ -7262,8 +7267,9 @@ static int nl80211_crypto_settings(struct
> cfg80211_registered_device *rdev,
> return -EINVAL;
> if (info->attrs[NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT])
> settings->control_port_no_encrypt = true;
> - } else
> + } else {
> settings->control_port_ethertype = cpu_to_be16(ETH_P_PAE);
> + }
>
> if (info->attrs[NL80211_ATTR_CIPHER_SUITES_PAIRWISE]) {
> void *data;
> @@ -7997,8 +8003,9 @@ static int nl80211_connect(struct sk_buff *skb, struct
> genl_info *info)
> if (!nl80211_valid_auth_type(rdev, connect.auth_type,
> NL80211_CMD_CONNECT))
> return -EINVAL;
> - } else
> + } else {
> connect.auth_type = NL80211_AUTHTYPE_AUTOMATIC;
> + }
>
> connect.privacy = info->attrs[NL80211_ATTR_PRIVACY];
>
> --
> 2.8.0.rc3.226.g39d4020
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 03/10] nl80211: Prefer ether_addr_copy
Hi All,
On Mon, May 30, 2016 at 12:52 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the checkpatch,pl to prefer ether_addr_copy
> over memcpy.
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
This looks right to me, but doesn't ether_addr_copy() have alignment
requirements? Could someone more familiar with that review these
changes to ensure they're met?
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index cd422bd..34b8fbe 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -3194,7 +3194,7 @@ static struct cfg80211_acl_data *parse_acl_data(struct
> wiphy *wiphy,
> return ERR_PTR(-ENOMEM);
>
> nla_for_each_nested(attr, info->attrs[NL80211_ATTR_MAC_ADDRS], tmp) {
> - memcpy(acl->mac_addrs[i].addr, nla_data(attr), ETH_ALEN);
> + ether_addr_copy(acl->mac_addrs[i].addr, nla_data(attr));
> i++;
> }
>
> @@ -5892,8 +5892,8 @@ static int nl80211_parse_random_mac(struct nlattr
> **attrs,
> if (!attrs[NL80211_ATTR_MAC] || !attrs[NL80211_ATTR_MAC_MASK])
> return -EINVAL;
>
> - memcpy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC]), ETH_ALEN);
> - memcpy(mac_addr_mask, nla_data(attrs[NL80211_ATTR_MAC_MASK]),
> ETH_ALEN);
> + ether_addr_copy(mac_addr, nla_data(attrs[NL80211_ATTR_MAC]));
> + ether_addr_copy(mac_addr_mask,
> nla_data(attrs[NL80211_ATTR_MAC_MASK]));
>
> /* don't allow or configure an mcast address */
> if (!is_multicast_ether_addr(mac_addr_mask) ||
> @@ -9405,8 +9405,7 @@ static int nl80211_parse_wowlan_tcp(struct
> cfg80211_registered_device *rdev,
> return -ENOMEM;
> cfg->src = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_SRC_IPV4]);
> cfg->dst = nla_get_in_addr(tb[NL80211_WOWLAN_TCP_DST_IPV4]);
> - memcpy(cfg->dst_mac, nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC]),
> - ETH_ALEN);
> + ether_addr_copy(cfg->dst_mac,
> nla_data(tb[NL80211_WOWLAN_TCP_DST_MAC]));
> if (tb[NL80211_WOWLAN_TCP_SRC_PORT])
> port = nla_get_u16(tb[NL80211_WOWLAN_TCP_SRC_PORT]);
> else
> --
> 2.8.0.rc3.226.g39d4020
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 3/3] nl80211: Various checkpatch.pl fixes
Hi Kirtika, On Mon, May 30, 2016 at 10:44 AM, Kirtika Ruchandani <kirtika.ruchand...@gmail.com> wrote: >>> There's too much stuff here to quickly review, it'd be nice if you >>> could split this up into patches that do the following: > > Is it preferable to resend the whole patch-set or just patch 3/3 in > this one as a separate set? Re-send the entire set, make sure you mark it as v2, include a changelog in the cover letter or below the "---" in the patches themselves, and don't forget to include the reviewed-bys for any unchanged patches. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 3/3] nl80211: Various checkpatch.pl fixes
Hi Kirtika,
On Sun, May 29, 2016 at 1:31 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the following checkpatch.pl issues -
There's too much stuff here to quickly review, it'd be nice if you
could split this up into patches that do the following:
Space issues:
> - space preferred around that
> - no space needed after cast.
> - Alignment should match open parenthesis
> - suspect code indent for conditional statements
> - Statements should start on a tabstop
Braces:
> - braces {} should be used on all arms of this statement
Multiple assignments:
> - multiple assignments should be avoided
ether_addr_copy:
> - prefer ether_addr_copy over memcpy
Spelling:
> - correct spelling - 'implementation'
NULL comparisons:
> - comparison to NULL could be written as !foo
kcalloc vs kzalloc:
> - prefer kcalloc over kzalloc with multiply
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/3] nl80211: Fix checkpatch warnings about blank lines
Hi All,
On Sun, May 29, 2016 at 1:31 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the following checkpatch.pl issues -
> - Please don't use multiple blank lines
> - Blank lines aren't necessary before a close brace
> - Missing a blank line after declarations
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
Looks sensible to me
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 22 ++
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 50a0de0..cd422bd 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -167,6 +167,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct
> nlattr **attrs)
>
> if (attrs[NL80211_ATTR_IFINDEX]) {
> int ifindex = nla_get_u32(attrs[NL80211_ATTR_IFINDEX]);
> +
> netdev = __dev_get_by_index(netns, ifindex);
> if (netdev) {
> if (netdev->ieee80211_ptr)
> @@ -730,6 +731,7 @@ static int nl80211_parse_key_new(struct nlattr *key,
> struct key_parse *k)
>
> if (tb[NL80211_KEY_DEFAULT_TYPES]) {
> struct nlattr *kdt[NUM_NL80211_KEY_DEFAULT_TYPES];
> +
> err = nla_parse_nested(kdt, NUM_NL80211_KEY_DEFAULT_TYPES - 1,
>tb[NL80211_KEY_DEFAULT_TYPES],
>nl80211_key_default_policy);
> @@ -1381,6 +1383,7 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
> rdev->ops->get_antenna) {
> u32 tx_ant = 0, rx_ant = 0;
> int res;
> +
> res = rdev_get_antenna(rdev, _ant, _ant);
> if (!res) {
> if (nla_put_u32(msg,
> @@ -2111,7 +2114,6 @@ static int nl80211_set_wds_peer(struct sk_buff *skb,
> struct genl_info *info)
> return rdev_set_wds_peer(rdev, dev, bssid);
> }
>
> -
> static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
> {
> struct cfg80211_registered_device *rdev;
> @@ -2244,6 +2246,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb,
> struct genl_info *info)
> if (info->attrs[NL80211_ATTR_WIPHY_ANTENNA_TX] &&
> info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]) {
> u32 tx_ant, rx_ant;
> +
> if ((!rdev->wiphy.available_antennas_tx &&
> !rdev->wiphy.available_antennas_rx) ||
> !rdev->ops->set_antenna)
> @@ -2910,6 +2913,7 @@ static int nl80211_get_key(struct sk_buff *skb, struct
> genl_info *info)
> pairwise = !!mac_addr;
> if (info->attrs[NL80211_ATTR_KEY_TYPE]) {
> u32 kt = nla_get_u32(info->attrs[NL80211_ATTR_KEY_TYPE]);
> +
> if (kt >= NUM_NL80211_KEYTYPES)
> return -EINVAL;
> if (kt != NL80211_KEYTYPE_GROUP &&
> @@ -3949,7 +3953,6 @@ static int nl80211_dump_station(struct sk_buff *skb,
> sta_idx++;
> }
>
> -
> out:
> cb->args[2] = sta_idx;
> err = skb->len;
> @@ -4742,7 +4745,6 @@ static int nl80211_dump_mpath(struct sk_buff *skb,
> path_idx++;
> }
>
> -
> out:
> cb->args[2] = path_idx;
> err = skb->len;
> @@ -5032,7 +5034,6 @@ static int nl80211_req_set_reg(struct sk_buff *skb,
> struct genl_info *info)
> enum nl80211_user_reg_hint_type user_reg_hint_type;
> u32 owner_nlportid;
>
> -
> /* You should only get this when cfg80211 hasn't yet initialized
> * completely when built-in to the kernel right between the time
> * window between nl80211_init() and regulatory_init(), if that is
> @@ -5240,7 +5241,6 @@ do {
> \
> } \
> } while (0)
>
> -
> if (!info->attrs[NL80211_ATTR_MESH_CONFIG])
> return -EINVAL;
> if (nla_parse_nested(tb, NL80211_MESHCONF_ATTR_MAX,
> @@ -5388,7 +5388,6 @@ static int nl80211_parse_mesh_setup(struct genl_info
> *info,
> IEEE80211_PATH_METRIC_VENDOR :
> IEEE80211_PATH_METRIC_AIRTIME;
>
> -
> if (tb[NL80211_MESH_SETUP_IE]) {
> struct nlattr *ieattr =
> tb
Re: [PATCH 1/3] nl80211: Fix checkpatch warnings
Hi All,
On Sun, May 29, 2016 at 1:30 PM, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> This patch fixes the following checkpatch.pl warnings about
> comments in nl80211.c :
> - networking block comments don't use an empty '/*' line
> - block comments use a trailing '*/' on a separate line
>
> Signed-off-by: Kirtika Ruchandani <kirtika.ruchand...@gmail.com>
The change and logic behind it are sound, so it gets my:
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
however I'm concerned that this file is a deliberate exception to the
networking comment rules.
Johannes?
Thanks,
Julian Calaby
> ---
> net/wireless/nl80211.c | 129
> +
> 1 file changed, 45 insertions(+), 84 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index d759901..50a0de0 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -196,8 +196,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct
> nlattr **attrs)
> return rdev;
> }
>
> -/*
> - * This function returns a pointer to the driver
> +/* This function returns a pointer to the driver
> * that the genl_info item that is passed refers to.
> *
> * The result of this can be a PTR_ERR and hence must
> @@ -1624,8 +1623,7 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
> goto nla_put_failure;
>
> features = rdev->wiphy.features;
> - /*
> -* We can only add the per-channel limit information if the
> + /* We can only add the per-channel limit information if the
> * dump is split, otherwise it makes it too big. Therefore
> * only advertise it in that case.
> */
> @@ -1646,8 +1644,7 @@ static int nl80211_send_wiphy(struct
> cfg80211_registered_device *rdev,
> rdev->wiphy.max_acl_mac_addrs))
> goto nla_put_failure;
>
> - /*
> -* Any information below this point is only available to
> + /* Any information below this point is only available to
> * applications that can deal with it being split. This
> * helps ensure that newly added capabilities don't break
> * older tools by overrunning their buffers.
> @@ -1847,8 +1844,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb,
> struct netlink_callback *cb)
> cb->nlh->nlmsg_seq,
> NLM_F_MULTI, state);
> if (ret < 0) {
> - /*
> -* If sending the wiphy data didn't fit
> (ENOBUFS
> + /* If sending the wiphy data didn't fit
> (ENOBUFS
> * or EMSGSIZE returned), this SKB is still
> * empty (so it's not too big because another
> * wiphy dataset is already in the skb) and
> @@ -1937,8 +1933,7 @@ static int parse_txq_params(struct nlattr *tb[],
>
> static bool nl80211_can_set_dev_channel(struct wireless_dev *wdev)
> {
> - /*
> -* You can only set the channel explicitly for WDS interfaces,
> + /* You can only set the channel explicitly for WDS interfaces,
> * all others have their channel managed via their respective
> * "establish a connection" command (connect, join, ...)
> *
> @@ -2131,8 +2126,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb,
> struct genl_info *info)
>
> ASSERT_RTNL();
>
> - /*
> -* Try to find the wiphy and netdev. Normally this
> + /* Try to find the wiphy and netdev. Normally this
> * function shouldn't need the netdev, but this is
> * done for backward compatibility -- previously
> * setting the channel was done per wiphy, but now
> @@ -2162,8 +2156,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb,
> struct genl_info *info)
> } else
> wdev = netdev->ieee80211_ptr;
>
> - /*
> -* end workaround code, by now the rdev is available
> + /* end workaround code, by now the rdev is available
> * and locked, and wdev may or may not be NULL.
> */
>
> @@ -2260,7 +2253,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb,
> struct genl_info *info)
> rx_ant =
> nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]);
>
> /* reject antenna
Re: [PATCH 2/2] rtlwifi: Fix reusable codes in core.c
Hi Kalle, On Sat, Apr 16, 2016 at 4:25 AM, Kalle Valo <kv...@codeaurora.org> wrote: > Byeoungwook Kim <quddnr...@gmail.com> writes: > >> rtl_*_delay() functions were reused same codes about addr variable. >> So i have converted to rtl_addr_delay() from code about addr variable. >> >> Signed-off-by: Byeoungwook Kim <quddnr...@gmail.com> >> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > > Doesn't apply: > > Applying: rtlwifi: Fix reusable codes in core.c > fatal: sha1 information is lacking or useless > (drivers/net/wireless/realtek/rtlwifi/core.c). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 rtlwifi: Fix reusable codes in core.c > > Please rebase and resend. This one is already applied in some form. I thought I'd listed it in my big list of superseded patches, however I must have missed it. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()
Hi Kalle, On Fri, Apr 15, 2016 at 10:09 PM, Kalle Valo <kv...@codeaurora.org> wrote: > Julian Calaby <julian.cal...@gmail.com> writes: > >> Hi Kalle, >> >> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring >> <elfr...@users.sourceforge.net> wrote: >>> From: Markus Elfring <elfr...@users.sourceforge.net> >>> Date: Fri, 1 Jan 2016 19:09:32 +0100 >>> >>> Replace an explicit initialisation for one local variable at the beginning >>> by a conditional assignment. >>> >>> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> >> >> This looks sane to me. >> >> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > > Before I commit I'll just change the commit title to: > > ath9k_htc: Replace a variable initialisation by an assignment in > ath9k_htc_set_channel() Sounds good to me. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] ath9k: remove duplicate assignment of variable ah
Hi All,
On Sun, Apr 10, 2016 at 9:25 PM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> ah is written twice with the same value, remove one of the
> redundant assignments to ah.
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
Looks right to me.
Signed-off-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
> ---
> drivers/net/wireless/ath/ath9k/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/init.c
> b/drivers/net/wireless/ath/ath9k/init.c
> index 77ace8d..fb702c4 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -477,7 +477,7 @@ static void ath9k_eeprom_request_cb(const struct firmware
> *eeprom_blob,
> static int ath9k_eeprom_request(struct ath_softc *sc, const char *name)
> {
> struct ath9k_eeprom_ctx ec;
> - struct ath_hw *ah = ah = sc->sc_ah;
> + struct ath_hw *ah = sc->sc_ah;
> int err;
>
> /* try to load the EEPROM content asynchronously */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()
Hi Kalle,
On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
<elfr...@users.sourceforge.net> wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 19:09:32 +0100
>
> Replace an explicit initialisation for one local variable at the beginning
> by a conditional assignment.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
This looks sane to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> index a680a97..30bd59e 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> @@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv
> *priv,
> struct ieee80211_conf *conf = >hw->conf;
> bool fastcc;
> struct ieee80211_channel *channel = hw->conf.chandef.chan;
> - struct ath9k_hw_cal_data *caldata = NULL;
> + struct ath9k_hw_cal_data *caldata;
> enum htc_phymode mode;
> __be16 htc_mode;
> u8 cmd_rsp;
> @@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv
> *priv,
> priv->ah->curchan->channel,
> channel->center_freq, conf_is_ht(conf), conf_is_ht40(conf),
> fastcc);
> -
> - if (!fastcc)
> - caldata = >caldata;
> -
> + caldata = fastcc ? NULL : >caldata;
> ret = ath9k_hw_reset(ah, hchan, caldata, fastcc);
> if (ret) {
> ath_err(common,
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] brcmfmac: sdio: remove unused variable retry_limit
Hi All, On Mon, Mar 21, 2016 at 4:34 AM, Colin King <colin.k...@canonical.com> wrote: > From: Colin Ian King <colin.k...@canonical.com> > > retry_limit has never been used during the life of this driver, so > we may as well remove it as it is redundant. > > Signed-off-by: Colin Ian King <colin.k...@canonical.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index 43fd3f4..cd92ba7 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -535,9 +535,6 @@ static int qcount[NUMPRIO]; > > #define RETRYCHAN(chan) ((chan) == SDPCM_EVENT_CHANNEL) > > -/* Retry count for register access failures */ > -static const uint retry_limit = 2; > - > /* Limit on rounding up frames */ > static const uint max_roundup = 512; > > -- > 2.7.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH][V3] mt7601u: do not free dma_buf when ivp allocation fails
Hi,
On Fri, Feb 26, 2016 at 10:24 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> If the allocation of ivp fails the error handling attempts to
> free an uninitialized dma_buf; this data structure just contains
> garbage on the stack, so the freeing will cause issues when the
> urb, buf and dma fields are free'd. Fix this by not free'ing the
> dma_buf if the ivp allocation fails.
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> ---
> drivers/net/wireless/mediatek/mt7601u/mcu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> index fbb1986..91c4b34 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> @@ -362,7 +362,9 @@ mt7601u_upload_firmware(struct mt7601u_dev *dev, const
> struct mt76_fw *fw)
> int i, ret;
>
> ivb = kmemdup(fw->ivb, sizeof(fw->ivb), GFP_KERNEL);
> - if (!ivb || mt7601u_usb_alloc_buf(dev, MCU_FW_URB_SIZE, _buf)) {
> + if (!ivb)
> + return -ENOMEM;
> + if (mt7601u_usb_alloc_buf(dev, MCU_FW_URB_SIZE, _buf)) {
> ret = -ENOMEM;
> goto error;
> }
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH][V2] mt7601u: do not free dma_buf when ivp allocation fails
Hi Colin,
On Fri, Feb 26, 2016 at 10:09 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> If the allocation of ivp fails the error handling attempts to
> free an uninitialized dma_buf; this data structure just contains
> garbage on the stack, so the freeing will cause issues when the
> urb, buf and dma fields are free'd. Fix this by not free'ing the
> dma_buf if the ivp allocation fails.
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
> drivers/net/wireless/mediatek/mt7601u/mcu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> index fbb1986..70e4b5e 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> @@ -362,10 +362,10 @@ mt7601u_upload_firmware(struct mt7601u_dev *dev, const
> struct mt76_fw *fw)
> int i, ret;
>
> ivb = kmemdup(fw->ivb, sizeof(fw->ivb), GFP_KERNEL);
> - if (!ivb || mt7601u_usb_alloc_buf(dev, MCU_FW_URB_SIZE, _buf)) {
> - ret = -ENOMEM;
> + if (!ivb)
> + return -ENOMEM;
> + if (mt7601u_usb_alloc_buf(dev, MCU_FW_URB_SIZE, _buf))
> goto error;
Are you sure this is right? Isn't ret unset here and consequently
returned at the end of the error label?
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mwifiex: Use to_delayed_work()
Hi All,
On Wed, Feb 17, 2016 at 11:33 PM, Amitoj Kaur Chawla
<amitoj1...@gmail.com> wrote:
> Introduce the use of to_delayed_work() helper function instead of open
> coding it with container_of()
>
> A simplified version of the Coccinelle semantic patch used to make
> this change is:
>
> //
> @@
> expression a;
> symbol work;
> @@
> - container_of(a, struct delayed_work, work)
> + to_delayed_work(a)
> //
>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1...@gmail.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
> ---
> drivers/net/wireless/marvell/mwifiex/11h.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/11h.c
> b/drivers/net/wireless/marvell/mwifiex/11h.c
> index 71a1b58..81c60d0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11h.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11h.c
> @@ -123,8 +123,7 @@ void mwifiex_11h_process_join(struct mwifiex_private
> *priv, u8 **buffer,
> void mwifiex_dfs_cac_work_queue(struct work_struct *work)
> {
> struct cfg80211_chan_def chandef;
> - struct delayed_work *delayed_work =
> - container_of(work, struct delayed_work, work);
> + struct delayed_work *delayed_work = to_delayed_work(work);
> struct mwifiex_private *priv =
> container_of(delayed_work, struct mwifiex_private,
> dfs_cac_work);
> @@ -289,8 +288,7 @@ int mwifiex_11h_handle_radar_detected(struct
> mwifiex_private *priv,
> void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
> {
> struct mwifiex_uap_bss_param *bss_cfg;
> - struct delayed_work *delayed_work =
> - container_of(work, struct delayed_work, work);
> + struct delayed_work *delayed_work = to_delayed_work(work);
> struct mwifiex_private *priv =
> container_of(delayed_work, struct mwifiex_private,
> dfs_chan_sw_work);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] ath10k: fix erroneous return value
Hi All,
On Thu, Feb 11, 2016 at 3:58 AM, Anton Protopopov
<a.s.protopo...@gmail.com> wrote:
> The ath10k_pci_hif_exchange_bmi_msg() function may return the positive
> value EIO instead of -EIO in case of error.
>
> Signed-off-by: Anton Protopopov <a.s.protopo...@gmail.com>
This looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> ---
> drivers/net/wireless/ath/ath10k/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c
> b/drivers/net/wireless/ath/ath10k/pci.c
> index ee925c6..526acde 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1756,7 +1756,7 @@ static int ath10k_pci_hif_exchange_bmi_msg(struct
> ath10k *ar,
> DMA_FROM_DEVICE);
> ret = dma_mapping_error(ar->dev, resp_paddr);
> if (ret) {
> - ret = EIO;
> + ret = -EIO;
> goto err_req;
> }
>
> --
> 2.6.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 8/9] rfkill: Userspace control for airplane mode
Hi All, On Wed, Feb 10, 2016 at 8:36 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote: > Provide an interface for the airplane-mode indicator be controlled from > userspace. User has to first acquire the control through > RFKILL_OP_AIRPLANE_MODE_ACQUIRE and keep the fd open for the whole time > it wants to be in control of the indicator. Closing the fd or using > RFKILL_OP_AIRPLANE_MODE_RELEASE restores the default policy. > > To change state of the indicator, the RFKILL_OP_AIRPLANE_MODE_CHANGE > operation is used, passing the value on "struct rfkill_event.soft". If > the caller has not acquired the airplane-mode control beforehand, the > operation fails. > > Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com> This looks sane to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/9] rfkill: Remove extra blank line
Hi João,
On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
> Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com>
Looks sane to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> ---
> net/rfkill/core.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index ffbc375..56d79cb 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -455,7 +455,6 @@ bool rfkill_get_global_sw_state(const enum rfkill_type
> type)
> }
> #endif
>
> -
> bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
> {
> unsigned long flags;
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 1/9] rfkill: Improve documentation language
Hi João,
On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
> Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com>
Looks right to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> ---
> net/rfkill/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index a805831..ffbc375 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -282,8 +282,8 @@ static void rfkill_set_block(struct rfkill *rfkill, bool
> blocked)
> spin_lock_irqsave(>lock, flags);
> if (err) {
> /*
> -* Failed -- reset status to _prev, this may be different
> -* from what set set _PREV to earlier in this function
> +* Failed -- reset status to _PREV, which may be different
> +* from what we have set _PREV to earlier in this function
> * if rfkill_set_sw_state was invoked.
> */
> if (rfkill->state & RFKILL_BLOCK_SW_PREV)
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 3/9] rfkill: Point to the correct deprecated doc location
Hi João, On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote: > The "claim" sysfs interface has been removed, so its documentation now > lives in the "removed" folder. > > Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com> Looks right to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > --- > Documentation/ABI/stable/sysfs-class-rfkill | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/Documentation/ABI/stable/sysfs-class-rfkill > b/Documentation/ABI/stable/sysfs-class-rfkill > index 097f522..e51571e 100644 > --- a/Documentation/ABI/stable/sysfs-class-rfkill > +++ b/Documentation/ABI/stable/sysfs-class-rfkill > @@ -2,8 +2,10 @@ rfkill - radio frequency (RF) connector kill switch support > > For details to this subsystem look at Documentation/rfkill.txt. > > -For the deprecated /sys/class/rfkill/*/state and > -/sys/class/rfkill/*/claim knobs of this interface look in > +For the deprecated /sys/class/rfkill/*/claim knobs of this interface look in > +Documentation/ABI/removed/sysfs-class-rfkill. > + > +For the deprecated /sys/class/rfkill/*/state knobs of this interface look in > Documentation/ABI/obsolete/sysfs-class-rfkill. > > What: /sys/class/rfkill > -- > 2.5.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 8/9] rfkill: Userspace control for airplane mode
Hi João,
On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
> Provide an interface for the airplane-mode indicator be controlled from
> userspace. User has to first acquire the control through
> RFKILL_OP_AIRPLANE_MODE_ACQUIRE and keep the fd open for the whole time
> it wants to be in control of the indicator. Closing the fd or using
> RFKILL_OP_AIRPLANE_MODE_RELEASE restores the default policy.
>
> To change state of the indicator, the RFKILL_OP_AIRPLANE_MODE_CHANGE
> operation is used, passing the value on "struct rfkill_event.soft". If
> the caller has not acquired the airplane-mode control beforehand, the
> operation fails.
>
> Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com>
> ---
> Documentation/rfkill.txt| 10 ++
> include/uapi/linux/rfkill.h | 3 +++
> net/rfkill/core.c | 47
> ++---
> 3 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index fb11547..8067701 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -1207,6 +1210,34 @@ static ssize_t rfkill_fop_write(struct file *file,
> const char __user *buf,
>
> mutex_lock(_global_mutex);
>
> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_ACQUIRE) {
> + if (rfkill_apm_owned && !data->is_apm_owner) {
> + count = -EACCES;
> + } else {
> + rfkill_apm_owned = true;
> + data->is_apm_owner = true;
> + }
> + }
> +
> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) {
> + if (rfkill_apm_owned && !data->is_apm_owner) {
Are you sure this is correct?
In the case that airplane mode isn't owned, the
rfkill_apm_led_trigger_event() call will be a noop, so we should
arguably not be calling it.
Also, should we just fail silently if we're not the owner? I.e. what
does userspace learn from this op failing and is that useful?
> + count = -EACCES;
> + } else {
> + bool state =
> rfkill_global_states[RFKILL_TYPE_ALL].cur;
> +
> + rfkill_apm_owned = false;
> + data->is_apm_owner = false;
> + rfkill_apm_led_trigger_event(state);
> + }
> + }
> +
> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) {
> + if (rfkill_apm_owned && data->is_apm_owner)
> + rfkill_apm_led_trigger_event(ev.soft);
> + else
> + count = -EACCES;
> + }
> +
> if (ev.op == RFKILL_OP_CHANGE_ALL)
> rfkill_update_global_state(ev.type, ev.soft);
>
> @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode,
> struct file *file)
> struct rfkill_int_event *ev, *tmp;
>
> mutex_lock(_global_mutex);
> +
> + if (data->is_apm_owner) {
> + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
> +
> + rfkill_apm_owned = false;
> + data->is_apm_owner = false;
> + rfkill_apm_led_trigger_event(state);
Also, this code is duplicated from the _RELEASE op above. Would it
make sense to factor it out into a separate function?
> + }
> +
> list_del(>list);
> +
(extra line)
> mutex_unlock(_global_mutex);
>
> mutex_destroy(>mtx);
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 5/9] rfkill: Factor rfkill_global_states[].cur assignments
Hi João,
On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
> Factor all assignments to rfkill_global_states[].cur into a single
> function rfkill_update_global_state().
>
> Signed-off-by: João Paulo Rechi Vita <jprv...@endlessm.com>
Looks sane to me.
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
> ---
> net/rfkill/core.c | 38 +-
> 1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 56d79cb..8b96869 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -302,6 +302,19 @@ static void rfkill_set_block(struct rfkill *rfkill, bool
> blocked)
> rfkill_event(rfkill);
> }
>
> +static void rfkill_update_global_state(enum rfkill_type type, bool blocked)
> +{
> + int i;
> +
> + if (type != RFKILL_TYPE_ALL) {
> + rfkill_global_states[type].cur = blocked;
> + return;
> + }
> +
> + for (i = 0; i < NUM_RFKILL_TYPES; i++)
> + rfkill_global_states[i].cur = blocked;
> +}
> +
> #ifdef CONFIG_RFKILL_INPUT
> static atomic_t rfkill_input_disabled = ATOMIC_INIT(0);
>
> @@ -319,15 +332,7 @@ static void __rfkill_switch_all(const enum rfkill_type
> type, bool blocked)
> {
> struct rfkill *rfkill;
>
> - if (type == RFKILL_TYPE_ALL) {
> - int i;
> -
> - for (i = 0; i < NUM_RFKILL_TYPES; i++)
> - rfkill_global_states[i].cur = blocked;
> - } else {
> - rfkill_global_states[type].cur = blocked;
> - }
> -
> + rfkill_update_global_state(type, blocked);
> list_for_each_entry(rfkill, _list, node) {
> if (rfkill->type != type && type != RFKILL_TYPE_ALL)
> continue;
> @@ -1164,15 +1169,8 @@ static ssize_t rfkill_fop_write(struct file *file,
> const char __user *buf,
>
> mutex_lock(_global_mutex);
>
> - if (ev.op == RFKILL_OP_CHANGE_ALL) {
> - if (ev.type == RFKILL_TYPE_ALL) {
> - enum rfkill_type i;
> - for (i = 0; i < NUM_RFKILL_TYPES; i++)
> - rfkill_global_states[i].cur = ev.soft;
> - } else {
> - rfkill_global_states[ev.type].cur = ev.soft;
> - }
> - }
> + if (ev.op == RFKILL_OP_CHANGE_ALL)
> + rfkill_update_global_state(ev.type, ev.soft);
>
> list_for_each_entry(rfkill, _list, node) {
> if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
> @@ -1261,10 +1259,8 @@ static struct miscdevice rfkill_miscdev = {
> static int __init rfkill_init(void)
> {
> int error;
> - int i;
>
> - for (i = 0; i < NUM_RFKILL_TYPES; i++)
> - rfkill_global_states[i].cur = !rfkill_default_state;
> + rfkill_update_global_state(RFKILL_TYPE_ALL, !rfkill_default_state);
>
> error = class_register(_class);
> if (error)
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] rtlwifi: Fix reusable codes in core.c
Hi Byeoungwook,
On Wed, Feb 3, 2016 at 2:48 AM, Byeoungwook Kim <quddnr...@gmail.com> wrote:
> rtl_*_delay() functions were reused same codes about addr variable.
> So i have converted to rtl_addr_delay() from code about addr variable.
>
> Conditional codes in rtl_addr_delay() were improved in readability and
> performance by using switch codes.
>
> Signed-off-by: Byeoungwook Kim <quddnr...@gmail.com>
> ---
> drivers/net/wireless/realtek/rtlwifi/core.c | 48
> +++--
> 1 file changed, 18 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c
> b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 4ae421e..c1193d1 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -37,36 +37,34 @@
>
> void rtl_addr_delay(u32 addr)
> {
> - if (addr == 0xfe)
> + switch (addr) {
> + case 0xfe:
> mdelay(50);
> - else if (addr == 0xfd)
> + break;
> + case 0xfd:
> mdelay(5);
> - else if (addr == 0xfc)
> + break;
> + case 0xfc:
> mdelay(1);
> - else if (addr == 0xfb)
> + break;
> + case 0xfb:
> udelay(50);
> - else if (addr == 0xfa)
> + break;
> + case 0xfa:
> udelay(5);
> - else if (addr == 0xf9)
> + break;
> + case 0xf9:
> udelay(1);
> + break;
> + };
As you're introducing a case statement here, you could consolidate the
addresses that have the same delays, i.e.
case 0xfe:
case 0xfb:
mdelay(50);
break;
also, you should arguably be doing this cleanup in a separate patch, i.e.
1. Convert open coded instances to use this function (i.e. the changes
below this comment)
2. Improve the function
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
> void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32
> addr,
> u32 mask, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> udelay(1);
> @@ -76,18 +74,8 @@ EXPORT_SYMBOL(rtl_rfreg_delay);
>
> void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> udelay(1);
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] rtlwifi: Fix reusable codes in core.c
Hi ByeoungWook, On Wed, Feb 3, 2016 at 11:52 AM, ByeoungWook Kim <quddnr...@gmail.com> wrote: > Hi Julian, > > 0xfe and 0xfb was not same codes. > 0xfe is udelay(50). and 0xfb is mdelay(50). > It same code like udelay((n) * 1000). I'm clearly blind! Sorry about that! > but i agree with your answers of some parts. I think that i should divide > into separate patch. > Thanks for your assists! Not a problem! Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 2/2] rtlwifi: Fix reusable codes in core.c
Hi Byeoungwook,
On Wed, Feb 3, 2016 at 1:01 PM, Byeoungwook Kim <quddnr...@gmail.com> wrote:
> rtl_*_delay() functions were reused same codes about addr variable.
> So i have converted to rtl_addr_delay() from code about addr variable.
>
> Signed-off-by: Byeoungwook Kim <quddnr...@gmail.com>
> Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Just a note for the future, you have to explicitly be given a
reviewed-by, you can't just assume that someone who has made comments
on a patch has reviewed it.
In this case, I have reviewed it, so formally:
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> drivers/net/wireless/realtek/rtlwifi/core.c | 28
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c
> b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 05f432c..c1193d1 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -63,18 +63,8 @@ EXPORT_SYMBOL(rtl_addr_delay);
> void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32
> addr,
> u32 mask, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> udelay(1);
> @@ -84,18 +74,8 @@ EXPORT_SYMBOL(rtl_rfreg_delay);
>
> void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> udelay(1);
> --
> 2.5.0
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH 1/2] rtlwifi: Fix improve function 'rtl_addr_delay()' in core.c
Hi Byeounwook,
On Wed, Feb 3, 2016 at 12:59 PM, Byeoungwook Kim <quddnr...@gmail.com> wrote:
> Conditional codes in rtl_addr_delay() were improved in readability and
> performance by using switch codes.
>
> Signed-off-by: Byeoungwook Kim <quddnr...@gmail.com>
> Reported-by: Julian Calaby <julian.cal...@gmail.com>
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
Julian Calaby
> ---
> drivers/net/wireless/realtek/rtlwifi/core.c | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c
> b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 4ae421e..05f432c 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -37,18 +37,26 @@
>
> void rtl_addr_delay(u32 addr)
> {
> - if (addr == 0xfe)
> + switch (addr) {
> + case 0xfe:
> mdelay(50);
> - else if (addr == 0xfd)
> + break;
> + case 0xfd:
> mdelay(5);
> - else if (addr == 0xfc)
> + break;
> + case 0xfc:
> mdelay(1);
> - else if (addr == 0xfb)
> + break;
> + case 0xfb:
> udelay(50);
> - else if (addr == 0xfa)
> + break;
> + case 0xfa:
> udelay(5);
> - else if (addr == 0xf9)
> + break;
> + case 0xf9:
> udelay(1);
> + break;
> + };
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
> --
> 2.5.0
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mac80211: fix memory leak
Hi Sudip, On Mon, Feb 1, 2016 at 8:33 PM, Sudip Mukherjee <sudipm.mukher...@gmail.com> wrote: > On Mon, Feb 01, 2016 at 11:28:37AM +0200, Kalle Valo wrote: >> Sudip Mukherjee <sudipm.mukher...@gmail.com> writes: >> >> > On Mon, Feb 01, 2016 at 11:03:35AM +1100, Julian Calaby wrote: >> >> Hi Sudip, >> >> >> >> On Fri, Jan 29, 2016 at 8:49 PM, Sudip Mukherjee >> >> <sudipm.mukher...@gmail.com> wrote: >> >> > On error we jumped to the error label and returned the error code but we >> >> > missed releasing sinfo. >> >> > >> >> > Signed-off-by: Sudip Mukherjee <su...@vectorindia.org> >> >> >> >> Should the From: and Signed-off-by: email addresses be the same? >> > >> > I think 2 years back I had a long discussion with Greg about this and >> > since then I al submitting patches like this. A small summayg of the >> > problem from that discussion: >> > >> > "we have strict DMARC check for the corporate mail server. DMARC = >> > domain based message authentication. >> > So the mail i sent reached all the list subscriber from a different >> > server than our designated server, and as a result it is marked as spam >> > in many places and I have already received a few complaints regarding >> > that." >> >> You can add a separate "From:" line to the beginning of the commit log >> and git will use it then commiting the patch. I didn't find any >> documention but it's easy to do and should solve this. > > Documentation is not needed. :) > I have done that couple of time. > I will resend this patch with the extra From: line. Don't forget to include the Fixes: tag. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mac80211: fix memory leak
Hi Sudip,
On Fri, Jan 29, 2016 at 8:49 PM, Sudip Mukherjee
<sudipm.mukher...@gmail.com> wrote:
> On error we jumped to the error label and returned the error code but we
> missed releasing sinfo.
>
> Signed-off-by: Sudip Mukherjee <su...@vectorindia.org>
Should the From: and Signed-off-by: email addresses be the same?
> ---
> net/mac80211/sta_info.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 6c198e6..36e75c4 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -561,6 +561,7 @@ static int sta_info_insert_finish(struct sta_info *sta)
> __acquires(RCU)
> __cleanup_single_sta(sta);
> out_err:
> mutex_unlock(>sta_mtx);
> + kfree(sinfo);
> rcu_read_lock();
> return err;
> }
Looks sane to me. I must note that the bug this is fixing is only in
the mac80211-next tree.
Fixes: 5fe74014172d ("mac80211: avoid excessive stack usage in sta_info")
Reviewed-by: Julian Calaby <julian.cal...@gmail.com>
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] mac80211: fix memory leak
Hi Sudip, On Mon, Feb 1, 2016 at 3:25 PM, Sudip Mukherjee <sudipm.mukher...@gmail.com> wrote: > On Mon, Feb 01, 2016 at 11:03:35AM +1100, Julian Calaby wrote: >> Hi Sudip, >> >> On Fri, Jan 29, 2016 at 8:49 PM, Sudip Mukherjee >> <sudipm.mukher...@gmail.com> wrote: >> > On error we jumped to the error label and returned the error code but we >> > missed releasing sinfo. >> > >> > Signed-off-by: Sudip Mukherjee <su...@vectorindia.org> >> >> Should the From: and Signed-off-by: email addresses be the same? > > I think 2 years back I had a long discussion with Greg about this and > since then I al submitting patches like this. A small summayg of the > problem from that discussion: > > "we have strict DMARC check for the corporate mail server. DMARC = > domain based message authentication. > So the mail i sent reached all the list subscriber from a different > server than our designated server, and as a result it is marked as spam > in many places and I have already received a few complaints regarding > that." Ok, fair enough then. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH v2 net] nfc: use GFP_USER for user-controlled kmalloc
Hi Cong On Sat, Jan 30, 2016 at 6:46 AM, Eric Dumazet <eric.duma...@gmail.com> wrote: > On Fri, 2016-01-29 at 11:24 -0800, Cong Wang wrote: >> These two functions are called in sendmsg path, and the >> 'len' is passed from user-space, so we should not allow >> malicious users to OOM kernel on purpose. >> >> Reported-by: Dmitry Vyukov <dvyu...@google.com> >> Cc: Lauro Ramos Venancio <lauro.venan...@openbossa.org> >> Cc: Aloisio Almeida Jr <aloisio.alme...@openbossa.org> >> Cc: Samuel Ortiz <sa...@linux.intel.com> >> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com> >> --- > > Note that the issue is not OOM the kernel (as the allocation is > attempted even after your patch), but having a way to > spill stack traces in the syslog. > > Acked-by: Eric Dumazet <eduma...@google.com> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
Hi Cong, On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote: A commit message would be nice. A brief rundown of how this is called from userspace would be nice (I'm talking a single sentence here, e.g. "this is allocated when submitting a nfc packet") and what issue __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation failures.) > Reported-by: Dmitry Vyukov <dvyu...@google.com> > Cc: Lauro Ramos Venancio <lauro.venan...@openbossa.org> > Cc: Aloisio Almeida Jr <aloisio.alme...@openbossa.org> > Cc: Samuel Ortiz <sa...@linux.intel.com> > Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com> > --- > net/nfc/llcp_commands.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c > index 3621a90..5d94055 100644 > --- a/net/nfc/llcp_commands.c > +++ b/net/nfc/llcp_commands.c > @@ -729,7 +729,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 > ssap, u8 dsap, > if (local == NULL) > return -ENODEV; > > - msg_data = kzalloc(len, GFP_KERNEL); > + msg_data = kzalloc(len, GFP_USER | __GFP_NOWARN); > if (msg_data == NULL) > return -ENOMEM; Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
Hi Cong, On Wed, Jan 27, 2016 at 10:12 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote: > On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby <julian.cal...@gmail.com> > wrote: >> Hi Cong, >> >> On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote: >> >> A commit message would be nice. A brief rundown of how this is called >> from userspace would be nice (I'm talking a single sentence here, e.g. >> "this is allocated when submitting a nfc packet") and what issue >> __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation >> failures.) >> > > I thought it is obvious. ;) Keep in mind that $subject is one part of commit > message too, so there is a commit message although very short. > > I will add it. I know almost nothing about how the NFC subsystem works, and this looks like a potential security issue, so more information is better IMHO. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] brcmfmac: sdio: Increase the default timeouts a bit
Hi Arend, On Tue, Jan 26, 2016 at 2:39 AM, Arend van Spriel <aspr...@gmail.com> wrote: > On 25-01-16 12:06, Julian Calaby wrote: >> Hi Sjoerd, >> >> On Mon, Jan 25, 2016 at 9:47 PM, Sjoerd Simons >> <sjoerd.sim...@collabora.co.uk> wrote: >>> On a Radxa Rock2 board with a Ampak AP6335 (Broadcom 4339 core) it seems >>> the card responds very quickly most of the time, unfortunately during >>> initialisation it sometimes seems to take just a bit over 2 seconds to >>> respond. >>> >>> This results intialization failing with message like: >>> brcmf_c_preinit_dcmds: Retreiving cur_etheraddr failed, -52 >>> brcmf_bus_start: failed: -52 >>> brcmf_sdio_firmware_callback: dongle is not responding >>> >>> Increasing the timeout to allow for a bit more headroom allows the >>> card to initialize reliably. >>> >>> A quick search online after diagnosing/fixing this showed that Google >>> has a similar patch in their ChromeOS tree, so this doesn't seem >>> specific to the board I'm using. >>> >>> Signed-off-by: Sjoerd Simons <sjoerd.sim...@collabora.co.uk> >> >> Looks sane to me. >> >> Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > > Not really a cleanup patch :-p , but thanks for the review. I'm trying to review any "small" patch from (relatively) new people. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] brcmfmac: sdio: Increase the default timeouts a bit
Hi Sjoerd, On Mon, Jan 25, 2016 at 9:47 PM, Sjoerd Simons <sjoerd.sim...@collabora.co.uk> wrote: > On a Radxa Rock2 board with a Ampak AP6335 (Broadcom 4339 core) it seems > the card responds very quickly most of the time, unfortunately during > initialisation it sometimes seems to take just a bit over 2 seconds to > respond. > > This results intialization failing with message like: > brcmf_c_preinit_dcmds: Retreiving cur_etheraddr failed, -52 > brcmf_bus_start: failed: -52 > brcmf_sdio_firmware_callback: dongle is not responding > > Increasing the timeout to allow for a bit more headroom allows the > card to initialize reliably. > > A quick search online after diagnosing/fixing this showed that Google > has a similar patch in their ChromeOS tree, so this doesn't seem > specific to the board I'm using. > > Signed-off-by: Sjoerd Simons <sjoerd.sim...@collabora.co.uk> Looks sane to me. Reviewed-by: Julian Calaby <julian.cal...@gmail.com> > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index dd66143..75ac4bd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -45,8 +45,8 @@ > #include "chip.h" > #include "firmware.h" > > -#define DCMD_RESP_TIMEOUT msecs_to_jiffies(2000) > -#define CTL_DONE_TIMEOUT msecs_to_jiffies(2000) > +#define DCMD_RESP_TIMEOUT msecs_to_jiffies(2500) > +#define CTL_DONE_TIMEOUT msecs_to_jiffies(2500) > > #ifdef DEBUG > > -- > 2.7.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
Hi Markus, On Tue, Jan 5, 2016 at 7:29 PM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: >> That said, if you figure out some change that produces significant >> reductions in code or binary size on multiple architectures without >> making things more complicated, less readable or making the code or >> binary size larger, then by all means propose it. > > Are you looking also for "a proof" that such changes are worthwhile? It'd be better than "I think doing things this way is better", which is the hallmark of most of your patch sets. (Admittedly not this one, but this one is where the discussion is now, so that's where we're discussing it.) >> "This makes things smaller" carries much more weight than >> "I think this is better". > > Can the discussed implementation of a function like "rsi_send_mgmt_pkt" > become a bit smaller by the deletion of extra variable initialisations I'm talking in general. In this case you're asking people to review a patch which requires a lot of careful review for a fairly minor improvement. I must also note that you haven't CC'd the people who wrote this driver, so it's possible that the only people who have reviewed it aren't experts in the code. The patches you sent recently which moved labels into if statements were a clear case of "I think this is better" where any actual benefit from the changes was eclipsed by the style and readability issues they introduced. >> Almost all of the changes you've proposed that have seen any >> discussion whatsoever fall into the latter category. > > Thanks for your interesting feedback. No problem. > Can a further constructive dialogue evolve from the presented information? Part of the issue here is that you don't seem to be listening to the discussion of your patches, or if you are, you're not significantly changing your approach or attitude in response. Every time you send a set of patches, there are legitimate issues which people raise, and every time they are discussed, you assert that your patches improve things and seem to ignore the concerns people raise. I've seen this same pattern of discussion here with these patches, with your patches to move labels into if statements, with the patches you sent late June last year, your patches to remove conditions before kfree() and friends, etc. You need to change you attitude: just because you can see some benefit from your patches doesn't mean others do and it doesn't mean that they're willing to accept them. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
Hi Markus, On Mon, Jan 4, 2016 at 11:33 PM, SF Markus Elfring <elfr...@users.sourceforge.net> wrote: >>> May I resend a consistent patch series for the source file >>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future? >> >> If you were sending checkpatch.pl fixes that would be easier to deal with > > Does this feedback mean that you would accept any more suggestions around > source code updates which are derived from recommendations of this script? A good rule of thumb here would be that if people start complaining about a particular type of change, stop sending them. Another good rule of thumb is to try to "rock the boat" on coding style and conventions as little as possible. Just because it's possible doesn't mean that people want to do it. That said, if you figure out some change that produces significant reductions in code or binary size on multiple architectures without making things more complicated, less readable or making the code or binary size larger, then by all means propose it. "This makes things smaller" carries much more weight than "I think this is better". Almost all of the changes you've proposed that have seen any discussion whatsoever fall into the latter category. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc
Hi,
On Tue, Dec 22, 2015 at 3:47 AM, Nicholas Mc Guire <hof...@osadl.org> wrote:
> This is an API consolidation only. The use of kmalloc + memset to 0
> is equivalent to kzalloc.
>
> Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
> ---
>
> Found by coccinelle script (relaxed version of
> scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
>
> Patch was compile tested with: x86_64_defconfig +
> CONFIG_WL12XX=m (implies CONFIG_WLCORE=m)
>
> Patch is against linux-next (localversion-next is -next-20151221)
>
> drivers/net/wireless/ti/wlcore/main.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/main.c
> b/drivers/net/wireless/ti/wlcore/main.c
> index ec7f6af..dfc49bf 100644
> --- a/drivers/net/wireless/ti/wlcore/main.c
> +++ b/drivers/net/wireless/ti/wlcore/main.c
> @@ -838,7 +838,7 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
>
> wl1271_info("Reading FW panic log");
>
> - block = kmalloc(wl->fw_mem_block_size, GFP_KERNEL);
> + block = kzalloc(wl->fw_mem_block_size, GFP_KERNEL);
> if (!block)
> return;
>
> @@ -885,7 +885,6 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
> goto out;
> }
> - memset(block, 0, wl->fw_mem_block_size);
I don't think you can't remove this line. It appears that the loop
this is part of resets block to be all zero, reads a chunk of data in,
then operates on it. I'm guessing that the code after the following
line expects that there isn't any data left over from previous runs
through the loop.
> ret = wlcore_read_hwaddr(wl, addr, block,
> wl->fw_mem_block_size, false);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc
Hi Nicholas,
On Tue, Dec 22, 2015 at 6:29 PM, Nicholas Mc Guire <der.h...@hofr.at> wrote:
> On Tue, Dec 22, 2015 at 09:56:10AM +1100, Julian Calaby wrote:
>> Hi,
>>
>> On Tue, Dec 22, 2015 at 3:47 AM, Nicholas Mc Guire <hof...@osadl.org> wrote:
>> > This is an API consolidation only. The use of kmalloc + memset to 0
>> > is equivalent to kzalloc.
>> >
>> > Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
>> > ---
>> >
>> > Found by coccinelle script (relaxed version of
>> > scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
>> >
>> > Patch was compile tested with: x86_64_defconfig +
>> > CONFIG_WL12XX=m (implies CONFIG_WLCORE=m)
>> >
>> > Patch is against linux-next (localversion-next is -next-20151221)
>> >
>> > drivers/net/wireless/ti/wlcore/main.c | 3 +--
>> > 1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/ti/wlcore/main.c
>> > b/drivers/net/wireless/ti/wlcore/main.c
>> > index ec7f6af..dfc49bf 100644
>> > --- a/drivers/net/wireless/ti/wlcore/main.c
>> > +++ b/drivers/net/wireless/ti/wlcore/main.c
>> > @@ -838,7 +838,7 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
>> >
>> > wl1271_info("Reading FW panic log");
>> >
>> > - block = kmalloc(wl->fw_mem_block_size, GFP_KERNEL);
>> > + block = kzalloc(wl->fw_mem_block_size, GFP_KERNEL);
>> > if (!block)
>> > return;
>> >
>> > @@ -885,7 +885,6 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
>> > goto out;
>> > }
>> > - memset(block, 0, wl->fw_mem_block_size);
>>
>> I don't think you can't remove this line. It appears that the loop
>> this is part of resets block to be all zero, reads a chunk of data in,
>> then operates on it. I'm guessing that the code after the following
>> line expects that there isn't any data left over from previous runs
>> through the loop.
>>
> the rational for this being ok is thta the copy operation into block is:
> ret = wlcore_read_hwaddr(wl, addr, block,
> wl->fw_mem_block_size, false);
>
> this will end up in the .read methods where block should be completely
> overwritten (length == full block size), so within the loop if successful
> this should be correct - if not successful it would "goto out" witout
> using the content of block.
>
> Am I overlooking something here ?
It's quite possible I am. I didn't look at the implementation of
wlcore_read_hwaddr() so I'm only guessing that it could do partial
reads.
That said, if it does overwrite the entire buffer each time, then
there's no need to use kzalloc() to allocate the buffer in the first
place.
Either way, it needs a review from someone more familiar with the code.
Thanks,
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
