b43: do not stack-allocate pio rx/tx header and tail buffers (was: pull request: wireless-2.6 2009-10-08)

b43: do not stack-allocate pio rx/tx header and tail buffers (was: pull request: wireless-2.6 2009-10-08)

[Albert Herranz]

Michael,

> Wait for my ack before you apply random b43 patches
                                   ^^^^^^
> The patch needlessly moves huge chunks of crap, adds stupid comments, wastes memory.
                                  ^^^^^^^^^^^^^^       ^^^^^^^^^^^^^^^
> This touched a lot of crap for fixing something that
       ^^^^^^^^^^^^^^^^^^^^^

If using that strong language helps you get your anger out of you I'm fine with that. But I think it is a bit unfair.

I'm not arguing if the patch should have been immediately merged upstream or not without your ack (I'm probably more on your side here, as the patch was still being discussed on the ML).
The patch [1] may not be up to your quality standards or may not take into account other requirements (that you have not expressed nor on the ML nor on IRC) but I'm sure it's far from being "random", "move crap" or "add stupid comments". That's the unfair part.

The reason I posted the initial patch for review was because you kind of told me [2].

[20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches

Then I tried to address your concerns [3] and sent v2 (the patch merged).
In that patch I moved slightly struct b43_wl within b43.h to satisfy the dependencies generated by the addition of the new members to that struct (which was one of your concerns).
There was even a bit of public discussion about the relocation of the struct declaration [4].

After ~22 hours if I'm not mistaken (yes, less than 24...) John, who had previously expressed his intentions to merge the patch [5], picked it for wireless-2.6.
And a day later, more or less again, John did the GIT PULL request.

That's it. IMHO, even if the patch was (mistakenly) merged too early for your standards, it doesn't break anything (AFAIK) and delivers what promises.
And (yes!) it can still be modeled to perfection later.

My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.

Other than that, I'm happy to see that the side conflict generated seems to be in a good shape now.
It would have been a total waste otherwise.

Thanks,
Albert

PS: Sorry to generate a new post, but I wasn't directly CC'ed on this and I'm not subscribed to the MLs. And I did also want to express my opinion! :)

[1] http://git.kernel.org/?p=linux/kernel/git/linville/wireless-2.6.git;a=commitdiff;h=7e937c633f718e0916a294db7282c922c1bf3ce3
[2] http://bcm-specs.sipsolutions.net/irc-logs/bcm-specs.2009-10-01
[3] http://marc.info/?l=linux-wireless&m=125486249831043
[4] http://marc.info/?l=linux-wireless&m=125493883613139
[5] http://marc.info/?l=linux-wireless&m=125486386100505

Re: b43: do not stack-allocate pio rx/tx header and tail buffers (was: pull request: wireless-2.6 2009-10-08)

[Michael Buesch]

On Friday 09 October 2009 19:46:31 Albert Herranz wrote:
> I'm not arguing if the patch should have been immediately merged upstream or not without your ack (I'm probably more on your side here, as the patch was still being discussed on the ML).
> The patch [1] may not be up to your quality standards or may not take into account other requirements (that you have not expressed nor on the ML nor on IRC) but I'm sure it's far from being "random", "move crap" or "add stupid comments". That's the unfair part.
> 
> The reason I posted the initial patch for review was because you kind of told me [2].
> 
> [20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches

Yeah, but that doesn't mean that either hack is acceptable. It just means that my time is limited
and I added this non-issue (which I still think it is) to the very bottom of my priority list.

> After ~22 hours if I'm not mistaken (yes, less than 24...) John, who had previously expressed his intentions to merge the patch [5], picked it for wireless-2.6.
> And a day later, more or less again, John did the GIT PULL request.

My impression was, that if the maintainer rejects a patch and asks for a new version,
the upstream maintainer must not take the patch until the maintainer acked the new version.
It seems I was wrong, though.

> My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
> I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.

It _was_ immediately NAK'ed by me. I did not know that I need to NAK
every single new version of a patch explicitely.
I thought NAK-ing a patch would put it into some special state that only an explicit ACK could
take it out of.

-- 
Greetings, Michael.

Re: b43: do not stack-allocate pio rx/tx header and tail buffers

[Albert Herranz]

Michael Buesch wrote:
>> The reason I posted the initial patch for review was because you kind of told me [2].
>>
>> [20:06] <mb_> Anyway, I'm not going to fix this. If you need it fixed, please send patches
> 
> Yeah, but that doesn't mean that either hack is acceptable. It just means that my time is limited
> and I added this non-issue (which I still think it is) to the very bottom of my priority list.

The patch got elaborated and discussed publicly (successfully or not) by following your instructions.

>> My point here is that there's no reason for such strong words concerning the quality of the patch code. It's really not that bad for such wording.
>> I'm sure that if the patch was really crap it would have been immediately NAK'ed by you or by any sane maintainer.
> 
> It _was_ immediately NAK'ed by me. I did not know that I need to NAK
> every single new version of a patch explicitely.
> I thought NAK-ing a patch would put it into some special state that only an explicit ACK could
> take it out of.

We all sure had a communication issue here.

What you thought it was an (implicit) NAK for the _initial_ version of the patch, others took that as "fix-those-concerns-and-its-fine".
And the expressed concerns where addressed later in the merged patch, sub-optimally (not crappily).

Looking at your new "[PATCH] b43: Optimize PIO scratchbuffer usage" to address the changes introduced by the merged patch, the merged solution is not that _blatantly_ far from your solution.
The patch would have probably got there in one iteration if you have had the chance again to express your new concerns about v2.

I'm sure we can avoid this in the future by being a bit more explicit.

Thanks,
Albert