* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:00 [patch net-next] net: qcom/emac: fix a sizeof() typo Dan Carpenter
@ 2017-02-13 11:55 ` walter harms
2017-02-13 13:03 ` Timur Tabi
2017-02-13 14:00 ` Dan Carpenter
2017-02-13 15:43 ` Timur Tabi
` (3 subsequent siblings)
4 siblings, 2 replies; 10+ messages in thread
From: walter harms @ 2017-02-13 11:55 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Timur Tabi, netdev, linux-kernel, kernel-janitors
Am 13.02.2017 12:00, schrieb Dan Carpenter:
> We had intended to say "sizeof(u32)" but the "u" is missing.
> Fortunately, sizeof(32) is also 4, so the original code still works.
>
> Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading hardware registers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> index 0d9945fb79be..bbe24639aa5a 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> @@ -227,7 +227,7 @@ static void emac_get_regs(struct net_device *netdev,
>
> static int emac_get_regs_len(struct net_device *netdev)
> {
> - return EMAC_MAX_REG_SIZE * sizeof(32);
> + return EMAC_MAX_REG_SIZE * sizeof(u32);
> }
>
We have a function where the argument is ignored and the rest is const ?
emac_ethtool_get_regs_len seems the only user. So it would be fairly easy to
move that into that function.
@maintainer:
Is there a deeper logic behind this ?
re,
wh
> static const struct ethtool_ops emac_ethtool_ops = {
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:55 ` walter harms
@ 2017-02-13 13:03 ` Timur Tabi
2017-02-13 13:28 ` walter harms
2017-02-13 14:00 ` Dan Carpenter
1 sibling, 1 reply; 10+ messages in thread
From: Timur Tabi @ 2017-02-13 13:03 UTC (permalink / raw)
To: wharms, Dan Carpenter; +Cc: netdev, linux-kernel, kernel-janitors
walter harms wrote:
> We have a function where the argument is ignored and the rest is const ?
>
> emac_ethtool_get_regs_len seems the only user. So it would be fairly easy to
> move that into that function.
>
> @maintainer:
> Is there a deeper logic behind this ?
I don't understand the question.
The patch is valid (I have no idea how I made that mistake).
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 13:03 ` Timur Tabi
@ 2017-02-13 13:28 ` walter harms
2017-02-13 15:43 ` Timur Tabi
0 siblings, 1 reply; 10+ messages in thread
From: walter harms @ 2017-02-13 13:28 UTC (permalink / raw)
To: Timur Tabi; +Cc: netdev, linux-kernel, kernel-janitors
Am 13.02.2017 14:03, schrieb Timur Tabi:
> walter harms wrote:
>> We have a function where the argument is ignored and the rest is const ?
>>
>> emac_ethtool_get_regs_len seems the only user. So it would be fairly
>> easy to
>> move that into that function.
>>
>> @maintainer:
>> Is there a deeper logic behind this ?
>
> I don't understand the question.
The question is: why is a simple calculation const*const
separated into a function ?
re,
wh
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:55 ` walter harms
2017-02-13 13:03 ` Timur Tabi
@ 2017-02-13 14:00 ` Dan Carpenter
1 sibling, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-02-13 14:00 UTC (permalink / raw)
To: walter harms; +Cc: Timur Tabi, netdev, linux-kernel, kernel-janitors
On Mon, Feb 13, 2017 at 12:55:03PM +0100, walter harms wrote:
>
>
> Am 13.02.2017 12:00, schrieb Dan Carpenter:
> > We had intended to say "sizeof(u32)" but the "u" is missing.
> > Fortunately, sizeof(32) is also 4, so the original code still works.
> >
> > Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading hardware registers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> > index 0d9945fb79be..bbe24639aa5a 100644
> > --- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> > +++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> > @@ -227,7 +227,7 @@ static void emac_get_regs(struct net_device *netdev,
> >
> > static int emac_get_regs_len(struct net_device *netdev)
> > {
> > - return EMAC_MAX_REG_SIZE * sizeof(32);
> > + return EMAC_MAX_REG_SIZE * sizeof(u32);
> > }
> >
>
>
> We have a function where the argument is ignored and the rest is const ?
>
> emac_ethtool_get_regs_len seems the only user. So it would be fairly easy to
> move that into that function.
It's not the only user. We have to pass netdev because it's used as a
function pointer.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:00 [patch net-next] net: qcom/emac: fix a sizeof() typo Dan Carpenter
2017-02-13 11:55 ` walter harms
@ 2017-02-13 15:43 ` Timur Tabi
2017-02-13 15:57 ` walter harms
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Timur Tabi @ 2017-02-13 15:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev, linux-kernel, kernel-janitors
Dan Carpenter wrote:
> We had intended to say "sizeof(u32)" but the "u" is missing.
> Fortunately, sizeof(32) is also 4, so the original code still works.
>
> Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading hardware registers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Timur Tabi <timur@codeaurora.org>
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:00 [patch net-next] net: qcom/emac: fix a sizeof() typo Dan Carpenter
2017-02-13 11:55 ` walter harms
2017-02-13 15:43 ` Timur Tabi
@ 2017-02-13 15:57 ` walter harms
2017-02-13 16:23 ` Timur Tabi
2017-02-14 3:21 ` David Miller
4 siblings, 0 replies; 10+ messages in thread
From: walter harms @ 2017-02-13 15:57 UTC (permalink / raw)
To: kernel-janitors
Am 13.02.2017 16:43, schrieb Timur Tabi:
> walter harms wrote:
>> The question is: why is a simple calculation const*const
>> separated into a function ?
>
> This is a callback function. That's just how it's defined.
>
> It's rare, but there are drivers that use the parameter, like this one:
>
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c#n591
>
>
this is the problem:
I used: http://lxr.free-electrons.com/source/drivers/net/ibm_emac/ibm_emac_core.c?v=2.6.24#L1778
here is the patched function emac_get_regs_len used the calculate the length in emac_ethtool_get_regs_len
what is used as callback.
in
http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
is it now used directly.
sorry for the noise,
re,
wh
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:00 [patch net-next] net: qcom/emac: fix a sizeof() typo Dan Carpenter
` (2 preceding siblings ...)
2017-02-13 15:57 ` walter harms
@ 2017-02-13 16:23 ` Timur Tabi
2017-02-14 3:21 ` David Miller
4 siblings, 0 replies; 10+ messages in thread
From: Timur Tabi @ 2017-02-13 16:23 UTC (permalink / raw)
To: kernel-janitors
Ah yes, thet are two completely different network drivers, both called EMAC.
IBM's EMAC controller is not related at all to Qualcomm's EMAC driver. It's
just an unfortunate coincidence that both devices are called EMAC, and that's
why both drivers are also called EMAC.
walter harms wrote:
>
>
> Am 13.02.2017 16:43, schrieb Timur Tabi:
>> walter harms wrote:
>>> The question is: why is a simple calculation const*const
>>> separated into a function ?
>>
>> This is a callback function. That's just how it's defined.
>>
>> It's rare, but there are drivers that use the parameter, like this one:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c#n591
>>
>>
>
> this is the problem:
> I used: http://lxr.free-electrons.com/source/drivers/net/ibm_emac/ibm_emac_core.c?v=2.6.24#L1778
>
> here is the patched function emac_get_regs_len used the calculate the length in emac_ethtool_get_regs_len
> what is used as callback.
>
> in
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> is it now used directly.
>
> sorry for the noise,
>
> re,
> wh
>
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
2017-02-13 11:00 [patch net-next] net: qcom/emac: fix a sizeof() typo Dan Carpenter
` (3 preceding siblings ...)
2017-02-13 16:23 ` Timur Tabi
@ 2017-02-14 3:21 ` David Miller
4 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2017-02-14 3:21 UTC (permalink / raw)
To: dan.carpenter; +Cc: timur, netdev, linux-kernel, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 13 Feb 2017 14:00:22 +0300
> We had intended to say "sizeof(u32)" but the "u" is missing.
> Fortunately, sizeof(32) is also 4, so the original code still works.
>
> Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading hardware registers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
^ permalink raw reply [flat|nested] 10+ messages in thread