Am Freitag, den 12.06.2015, 14:09 -0400 schrieb Alan Stern: > On Fri, 12 Jun 2015, Stefan Koch wrote: > There is a lot of questionable material here. > > First of all, I agree with Krzysztof that having an "authorized" > attribute in each interface's sysfs directory would be simpler and > easier to use than having a bitmask of all authorized interfaces. OK I can provide a patch for it. But note that the mask allows to enable multiple interfaces at once. And the mechanism does enable all (multiple) interfaces first and then does start the driver probing for all interfaces. This mechanism is not possible without a mask. But the attribute may be useful for single interfaces that work alone. > > Secondly, the patches have not been carefully edited. There are > several misspelled words in comments and descriptions. And why does > patch 3/5 modify drivers/base/base.h and include/linux/device.h? > It's needed that bus_probe_device() is defined in hub.c. To start probing after authorize an interface. > Thirdly, what is the purpose of the mask_changed bit? The changelog > describes it as "a status bit to control a manual setting of the mask", > which is not very clear. _How_ does it control manual setting of the > mask? _Why_ does manual setting of the mask need to be controlled? > If someone sets the mask the bit get's TRUE. After setting a new configuration it is set back to FALSE. So you could check if it is needed to ensure correct mask setting (again). > Fourthly, it doesn't look like usb_set_configuration() does the right > thing. It should call usb_enable_interface() whether the interface is > authorized or not. > > Fifthly, the code style is awkward in several places. For instance, > it looks silly to do this (usb_alloc_dev in patch 2/5): > > dev->mask = 0; > > /* invert the mask. each bit of the mask is now TRUE. > * all interfaces should be allowed. */ > dev->mask = ~dev->mask; > > Not to mention that the accepted style for multi-line comments is > like this: > > /* > * This is a > * long comment. > */ > > Also, the kernel doesn't use CamelCase names like intfNr. > > All these issues will have to be fixed before the patches can be > accepted. > > Alan Stern > Thanks Stefan Koch -- 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
