[PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Okash Khawaja]

The PCI cache line size value was being compared against decimal values prefixed with 0x.

Fixed the literals to use the correct hex values.


Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
---
 drivers/net/wireless/adm8211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/adm8211.c b/drivers/net/wireless/adm8211.c
index 058fb4b..76c908f 100644
--- a/drivers/net/wireless/adm8211.c
+++ b/drivers/net/wireless/adm8211.c
@@ -1101,10 +1101,10 @@ static void adm8211_hw_init(struct ieee80211_hw *dev)
 		case  0x8:
 			reg |= (0x1 << 14);
 			break;
-		case 0x16:
+		case 0x10:
 			reg |= (0x2 << 14);
 			break;
-		case 0x32:
+		case 0x20:
 			reg |= (0x3 << 14);
 			break;
 		default:
-- 
1.9.1

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Kalle Valo]

Okash Khawaja <okash.khawaja-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> The PCI cache line size value was being compared against decimal values prefixed with 0x.
>
> Fixed the literals to use the correct hex values.
>
>
> Signed-off-by: Okash Khawaja <okash.khawaja-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

[...]

> @@ -1101,10 +1101,10 @@ static void adm8211_hw_init(struct ieee80211_hw *dev)
>  		case  0x8:
>  			reg |= (0x1 << 14);
>  			break;
> -		case 0x16:
> +		case 0x10:
>  			reg |= (0x2 << 14);
>  			break;
> -		case 0x32:
> +		case 0x20:
>  			reg |= (0x3 << 14);
>  			break;
>  		default:

Did you test this? How certain can we be that this doesn't break
anything?

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Okash Khawaja]

On Wed, May 06, 2015 at 07:59:04AM +0300, Kalle Valo wrote:
> Okash Khawaja <okash.khawaja@gmail.com> writes:
> 
> > The PCI cache line size value was being compared against decimal values prefixed with 0x.
> >
> > Fixed the literals to use the correct hex values.
> >
> >
> > Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> 
> [...]
> 
> > @@ -1101,10 +1101,10 @@ static void adm8211_hw_init(struct ieee80211_hw *dev)
> >  		case  0x8:
> >  			reg |= (0x1 << 14);
> >  			break;
> > -		case 0x16:
> > +		case 0x10:
> >  			reg |= (0x2 << 14);
> >  			break;
> > -		case 0x32:
> > +		case 0x20:
> >  			reg |= (0x3 << 14);
> >  			break;
> >  		default:
> 
> Did you test this? How certain can we be that this doesn't break
> anything?
> 

I didn't test it as that would require the hardware that I don't have at the moment. However, the value in `cline` is PCI cache line size, which is the CPU's cache line size. It is less likely for cache line sizes to be 22 or 50, and more likely for them to be 16 or 32. Also, as far as I understand (and I might be wrong here), cache line size is used for things like aligning DMA requests with CPU cache line, which improve performance but wouldn't break anything if the value doesn't match. In this case, we will fall through to the default case which leaves `reg` unchanged.

If there is a way to test it with a mock set up or if you still think we need to test this on real board, I'll be happy to try get the hardware. But I will need some guidance around that. Thanks.

> -- 
> Kalle Valo

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Kalle Valo]

Okash Khawaja <okash.khawaja@gmail.com> writes:

> On Wed, May 06, 2015 at 07:59:04AM +0300, Kalle Valo wrote:
>> Okash Khawaja <okash.khawaja@gmail.com> writes:
>> 
>> > The PCI cache line size value was being compared against decimal
>> > values prefixed with 0x.
>> > Fixed the literals to use the correct hex values.
>> >
>> > Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
>>  [...]
>> 
>> > @@ -1101,10 +1101,10 @@ static void adm8211_hw_init(struct
>> > ieee80211_hw *dev) case 0x8: reg |= (0x1 << 14); break; - case
>> > 0x16: + case 0x10: reg |= (0x2 << 14); break; - case 0x32: + case
>> > 0x20: reg |= (0x3 << 14); break; default:
>>  Did you test this? How certain can we be that this doesn't break
>> anything?
>> 
>
> I didn't test it as that would require the hardware that I don't have
> at the moment. However, the value in `cline` is PCI cache line size,
> which is the CPU's cache line size. It is less likely for cache line
> sizes to be 22 or 50, and more likely for them to be 16 or 32. Also,
> as far as I understand (and I might be wrong here), cache line size is
> used for things like aligning DMA requests with CPU cache line, which
> improve performance but wouldn't break anything if the value doesn't
> match. In this case, we will fall through to the default case which
> leaves `reg` unchanged.
>
> If there is a way to test it with a mock set up or if you still think
> we need to test this on real board, I'll be happy to try get the
> hardware. But I will need some guidance around that. Thanks.

I don't have any ideas how to test this as I think the hardware is
pretty rare nowadays but I think this is safe to commit, thanks for
clearing this up. BTW, what you wrote about would have been perferct in
the commit log itself.

-- 
Kalle Valo

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Okash Khawaja]

> On 11 May 2015, at 07:30, Kalle Valo <kvalo@codeaurora.org> wrote:
> 
> Okash Khawaja <okash.khawaja@gmail.com> writes:
> 
>>> On Wed, May 06, 2015 at 07:59:04AM +0300, Kalle Valo wrote:
>>> Okash Khawaja <okash.khawaja@gmail.com> writes:
>>> 
>>>> The PCI cache line size value was being compared against decimal
>>>> values prefixed with 0x.
>>>> Fixed the literals to use the correct hex values.
>>>> 
>>>> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
>>> [...]
>>> 
>>>> @@ -1101,10 +1101,10 @@ static void adm8211_hw_init(struct
>>>> ieee80211_hw *dev) case 0x8: reg |= (0x1 << 14); break; - case
>>>> 0x16: + case 0x10: reg |= (0x2 << 14); break; - case 0x32: + case
>>>> 0x20: reg |= (0x3 << 14); break; default:
>>> Did you test this? How certain can we be that this doesn't break
>>> anything?
>> 
>> I didn't test it as that would require the hardware that I don't have
>> at the moment. However, the value in `cline` is PCI cache line size,
>> which is the CPU's cache line size. It is less likely for cache line
>> sizes to be 22 or 50, and more likely for them to be 16 or 32. Also,
>> as far as I understand (and I might be wrong here), cache line size is
>> used for things like aligning DMA requests with CPU cache line, which
>> improve performance but wouldn't break anything if the value doesn't
>> match. In this case, we will fall through to the default case which
>> leaves `reg` unchanged.
>> 
>> If there is a way to test it with a mock set up or if you still think
>> we need to test this on real board, I'll be happy to try get the
>> hardware. But I will need some guidance around that. Thanks.
> 
> I don't have any ideas how to test this as I think the hardware is
> pretty rare nowadays but I think this is safe to commit, thanks for
> clearing this up. BTW, what you wrote about would have been perferct in
> the commit log itself.
> 
> -- 
> Kalle Valo

Sure, I'll create v2 of the patch with updated commit log. 

Since it's part of a patch set, do you want me to send both the patches in the patch set together as v2 or just this patch? 

Thanks

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Kalle Valo]

Okash Khawaja <okash.khawaja-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Sure, I'll create v2 of the patch with updated commit log. 
>
> Since it's part of a patch set, do you want me to send both the
> patches in the patch set together as v2 or just this patch?

Please resend the whole patchset as v2, less problems that way.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Jonas Gorski]

Hi,

On Mon, May 11, 2015 at 12:38 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Okash Khawaja <okash.khawaja@gmail.com> writes:
>
>> Sure, I'll create v2 of the patch with updated commit log.
>>
>> Since it's part of a patch set, do you want me to send both the
>> patches in the patch set together as v2 or just this patch?
>
> Please resend the whole patchset as v2, less problems that way.

If you are sending a v2 anyway, I'd suggest using decimal values for
the cases, as cache line sizes are usually given in decimal anyway. So
just drop the "0x" instead of converting the values ;-)


Jonas

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Okash Khawaja]

> On 11 May 2015, at 14:24, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> 
> Hi,
> 
>> On Mon, May 11, 2015 at 12:38 PM, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> Okash Khawaja <okash.khawaja-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> 
>>> Sure, I'll create v2 of the patch with updated commit log.
>>> 
>>> Since it's part of a patch set, do you want me to send both the
>>> patches in the patch set together as v2 or just this patch?
>> 
>> Please resend the whole patchset as v2, less problems that way.
> 
> If you are sending a v2 anyway, I'd suggest using decimal values for
> the cases, as cache line sizes are usually given in decimal anyway. So
> just drop the "0x" instead of converting the values ;-)
> 
> 
> Jonas

Jonas, I had already sent the updated patch by the time I received your email. Is there a convention around this?
Thanks--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] adm8211: fix the possible pci cache line sizes inside switch-case

[Jonas Gorski]

On 11.05.2015 19:48, Okash Khawaja wrote:
>> On 11 May 2015, at 14:24, Jonas Gorski <jogo@openwrt.org> wrote:
>>
>> Hi,
>>
>>> On Mon, May 11, 2015 at 12:38 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Okash Khawaja <okash.khawaja@gmail.com> writes:
>>>
>>>> Sure, I'll create v2 of the patch with updated commit log.
>>>>
>>>> Since it's part of a patch set, do you want me to send both the
>>>> patches in the patch set together as v2 or just this patch?
>>>
>>> Please resend the whole patchset as v2, less problems that way.
>>
>> If you are sending a v2 anyway, I'd suggest using decimal values for
>> the cases, as cache line sizes are usually given in decimal anyway. So
>> just drop the "0x" instead of converting the values ;-)
>>
>>
>> Jonas
> 
> Jonas, I had already sent the updated patch by the time I received your email. Is there a convention around this?

You did everything right, just gmail decided "helpfully" they were spam,
so I didn't see that you already sent the v2.

So ingore my comment unless you need to send a v3. It's only a minor
nitpick but nothing respin-worthy IMHO.


Jonas