On Tue, Oct 01, 2013 at 09:51:07AM +0200, Matthijs Kooijman wrote: > On Tue, Oct 01, 2013 at 10:05:17AM +0300, Dan Carpenter wrote: > > On Tue, Oct 01, 2013 at 01:21:28AM +0000, Paul Zimmerman wrote: > > > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > > > Sent: Monday, September 30, 2013 6:09 PM > > > > > > > > Yeah. I guess it's fine... I was going to suggest adding the + 1 in a > > > > different place but actually it doesn't matter. > > > > > > > > The key to understanding dwc2_set_param_host_channels() is that the > > > > "val" parameter is always -1. That means it always returns -EINVAL and > > > > the caller jumbles the error code in with some other error codes and > > > > then ignores any errors. :/ > > > > > > The intent of this was that the value can be overridden by the platform > > > code if required, in which case "val" would not be -1. However, to my > > > knowledge, no in-tree platforms do that, so I guess it would be fine to > > > redo this as you suggest below. We can always add it back if needed. > If we redo the dwc2_set_param_host_channels function, we should probably > redo _all_ of the dwc2_set_param_* functions, since they all do this. > Yeah... Generally one of the early steps in staging drivers is to delete as much code as possible. The rule in the kernel is to not include any functionality which doesn't have a user. > > Why are we even adding one to the number of channels that the hardware > > reports? > Because that's how the hardware register is defined. I presume it never > makes sense to have 0 host channels, this allows a range of 1-16 instead > of 0-15. > > In any case, for this particular problem, I would think that simply > making the host_channels field in dcw2_hw_params bigger is the better solution > here. E.g., something like: > > struct dwc2_hw_params { > ... > - unsigned host_channels:4; > + unsigned host_channels:5; > } > > Moving the +1 calculation reverts the code back to what it was before one of my > patches. I moved the +1 to this place, so that the off-by-one detail of > the hardware register is only known at the place where hardware > registers are read. All other code can simply assume that the > "host_channels" variable contains just that: the number of host channels > present. > > However, in that patch I apparently chose the wrong size for the > host_channels field, which I think should be problem to fix here. > Somehow I assumed that was fixed by the hardware, but I see now that you are right. Yes, making the definition larger is better than moving the + 1. Dinh, do you want to do that? The other option is that Matthijs could fix it and give you the Reported-by credit. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
