LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg KH <greg@kroah.com>
To: Rainer Weikusat <rainer.weikusat@sncag.com>
Cc: Rainer Weikusat <rweikusat@sncag.com>,
	torvalds@linux-foundation.org, bunk@stusta.de,
	linux-kernel@vger.kernel.org
Subject: Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
Date: Fri, 26 Jan 2007 09:48:28 -0800	[thread overview]
Message-ID: <20070126174828.GA28890@kroah.com> (raw)
In-Reply-To: <87y7nq131x.fsf@semtex.sncag.com>

On Fri, Jan 26, 2007 at 11:43:22AM +0100, Rainer Weikusat wrote:
> Greg KH <greg@kroah.com> writes:
> > On Thu, Jan 25, 2007 at 04:20:55PM +0100, Rainer Weikusat wrote:
> >> 2.6.19 introduced changes to the UHCI handling of interrupt URBs that
> >> caused at least some keyspan USB-to-serial converters to fail,
> 
> [...]
> 
> > I copied you when this patch was added to my tree,
> 
> The only thing I received was a 'this patch has been dropped from
> the MM-tree because something happened' message.

You were also copied on the patch when it was added to my tree.  Email
was sent to your rweikusat@sncag.com address.

> > as I had reworked it to be a bit more acceptable (no pointer
> > arithmatic, proper coding style, use the newer macros for endpoint
> > detection, etc.),
> 
> You have basically done a couple of (functionally) totally pointless
> changes, like
> 
> 	- not using iterator-style array traversals, but indexed ones
>           instead

Which is the better way to document exactly how you are walking through
all of the endpoints.

> 	- doing the calculation to determine the endpoint type in
>           three different places instead of in one place, because the
>           somewhat silly endpoint classification interfaces enforces
>           this

3 places?  You mean in the if call?

> 	- not using a single switch-statement but an if-else-cascade,
>           again due to limitations of that interface

This really isn't a problem.

> 	- replacing the while-loop with an identical for-loop

Again, this is the better way to walk through all of the endpoints for a
device.

> The net effect of these changes is that an optimizing compiler will
> have to work somewhat more to remove all the redundant stuff that
> was added.

This is initialization code, it's better to be obvious as to what you
are trying to do here than trying to do the compiler's job for it.
Especially as it is not a critical path by any means.

> As for 'proper coding style', the code conforms to what is
> documented as 'proper kernel coding style'. If this assumption of mine
> is incorrect, I'd be happy to hear about reasons why this would be so,
> to take them into account for eventual future patches.

There were some statements that had more than one on one line (I think
in a few usages of 'if'), but as I don't have the original patch handy,
I'll defer.

The _main_ coding style difference is that it was not obvious as to what
you were doing while walking the list of endpoints in the device.  That
is why I changed the code, to be more obvious to who ever looks at the
code next (most likely me in about 2 years...)

> > I this patch does not work, please let me know, and I will be glad
> > to work to fix it.
> 
> I think I'll just resend the working one in this case. But the logic
> appears to be identical, so I do not so a reason why it shouldn't.

Please work to see what is wrong with the existing patch.  Is there
anything that I can do to help you out?

thanks,

greg k-h

  reply	other threads:[~2007-01-26 17:49 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-25 15:20 Rainer Weikusat
2007-01-25 18:51 ` Greg KH
2007-01-26 10:43   ` Rainer Weikusat
2007-01-26 17:48     ` Greg KH [this message]
2007-01-28 13:34       ` Rainer Weikusat
2007-01-29  1:22         ` Oleg Verych
2007-01-29 23:43           ` Greg KH
2007-01-30 17:40             ` Rainer Weikusat
2007-02-04  4:39               ` Greg KH
2007-02-12 14:30                 ` Rainer Weikusat
2007-02-12 16:37                   ` Greg KH
2007-02-12 17:10                     ` Rainer Weikusat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070126174828.GA28890@kroah.com \
    --to=greg@kroah.com \
    --cc=bunk@stusta.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rainer.weikusat@sncag.com \
    --cc=rweikusat@sncag.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).