Hi, On Mon, Aug 22, 2016 at 05:50:25PM +0200, Hans de Goede wrote: > Hi, > > On 22-08-16 16:08, Bin Liu wrote: > >Hi, > > > >On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote: > >>Hi, > >> > >>On 19-08-16 23:25, Bin Liu wrote: > >>>Hi, > >>> > >>>On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote: > >>>>Hi All, > >>>> > >>>>Here is a patch series which implements run-time changing the dr-mode > >>>>of sunxi musb controllers through the (already existing) musb "mode" > >>>>sysfs attribute. > >>>> > >>>>This is useful on boards where there is no id pin, e.g. some tv-boxes > >>>>use the musb controller to get an extra usb A port without needing > >>>>a hub chip. Except for the missing id pin when using a usb A<->A cable > >>>>these ports can do peripheral mode just fine. This series makes it > >>>>possible to do e.g. this by doing echo "peripheral" > mode before > >>>>plugging in the usb A<->A cable. > >>> > >>>Well, this is an illegal usecase. A-A cable is invalid by USB Spec. > >> > >>Yet they exist and there are USB devices (e.g. harddisk enclosures) > >>which use a USB-A connector even though they are a device not > >>a host. I own several such devices myself. I agree that this is > >>wrong but these devices exist, typical case of reality trumping > >>the spec. > > > >I know those products exist, but they are different from yours. Those > >existing devices are usb peripheral-only, which never source vbus so do > >not hurt anything other than violate the USB Spec. But your usecase > >allows the device switching to host mode, which could damage the usb > >host on the other end of the connection due to vbus contention. > > Actually the sun4i phy driver avoids this by checking vbus-detect for > external vbus presence before enabling the boards own Vbus. > > More importantly, not supporting peripheral mode on musb attached to an > USB-A receptacle will not stop users from trying to use it and insert > an A<->A cable at which point the damage is already done. > > I understand why you dislike this, but we cannot stop a user > from trying this. So it is better to actually be prepared for this > (e.g. do the vbus-detect check we're already doing) then it is > to not support this. > > For example: > > Lets say we do not have vbus-detect and the user plugs in > an A<->A cable causing Vbus being driven from 2 sides. > > Not good, now we have current flowing between the 2 > power supplies and things are heating up. > > Now there are 2 possible follow-up scenarios: > > 1) We support peripheral mode, the user does > "echo peripheral > mode" in sysfs and we disable > the boards Vbus, no more current, things are cooling > down again before components burn up. > > 2) We do not support peripheral mode (which is what you're > suggesting), the user does "echo peripheral > mode" in sysfs, > nothing happens. Now we can only happen the user unplugs > the A<->A cable soon, instead of trying a bunch of other things. > > Neither is ideal, but do you see how not allowing peripheral > mode is not helping here ? As soon as the user plugs in > an A<->A cable bad things may happen, we cannot avoid that > from the software side. > > >>>With type-A receptacle the controller should be in host-only mode, > >>>switching to peripheral mode should not be allowed. > >> > >>Peripheral mode can still be quite useful, e.g. to do ethernet > >>over USB (some of these devices lack wired ethernet). > > > >For such usecase, a micro-AB connector should be used, or use two > >connectors - type-A and type-B which are mux'd to the same usb port. > > > >> > >>Also the manual of these devices typically instructs users to > >>use an A<->A cable for firmware updates, since the bootROM in > > > >(Nobody reads the manual, we all know it...) This does not prevent users > >to connect two hosts together then damage the hardware. So this design > >should not be allowed. > > Nothing prevents an user from doing that, as soon as they have > an A<->A cable they can easily do that, one can only hope they > will not do it. > > >>the SoC does not care anmd will happily put the device in > >>Allwinner's custom DFU mode using the USB-A connector. > >> > >>Although I agree with this being not ideal, the fact is that > >>an USB-A connector combined with a A<->A cable is electrically > >>100% identical to a USB-B connector with its id-pin not connected > >>(on the PCB side), which also happens see below. > > > >they are two different cables. A-B cable does not provide a chance to > >connect two hosts together, but A-A cable does. > > Right, but an A-B cable with its id-pin wrongly connected to ground > (some so called otg charging hubs do this) will cause the exact same issue. > > This is why usb-ports and especially otg ports should have current limiting > on their Vbus and why the sun4i phy driver checks vbus-detect not reporting > an external vbus before enabling Vbus. > > >>>>This series has both sun4i-usb-phy driver and sunxi-musb-glue changes, > >>>>both are necessary for the run-time changing to work, but they can be > >>>>merged independently without breaking anything. > >>>> > >>>>Changed in v2: > >>>> > >>>>After sleeping on it a night I realized that always passing port_mode = > >>>>DUAL_ROLE to the musb-core was wrong. There is a distintion between > >>>>the id-pin not working properly which we can work around with software > >>>>mode switching (and we want to register both the host and udc driver > >>>>in this case) vs cases where we really only want to register a host > >>>>(wifi module connected to musb soldered onto the PCB). > >>>> > >>>>So v2 of this series drops the > >>>>"musb: sunxi: Always register both host and udc when build with dual-role support" > >>>>patch. > >>>> > >>>>Instead systems which are dual-role capable, but lack an id-pin should use > >>>>dr_mode = "otg" in the dts file. There is one problem with this, some > >>>>systems are dual-role capable but use a female USB-A connector connected > >>>>to the musb controller. These should come up in host mode by default, > >>> > >>>This is not a problem. With a type-A connector, the dual-role controller > >>>should work in host-only mode. > >>>Role switching should only be allowed if an AB connector is used. > >> > >>Yet people may want to use it in peripheral mode, I strongly believe > >>that we should not be telling people what they can and cannot do > >>with their hardware. That is policy and the kernel does not set > > > >I agree with the policy, but we are responsible to tell people don't do > >something which is not correct. > > > >>such policy, that is up to userspace. OTOH the port should clearly > >>default to host mode hence the new property. > > > >Due to the problem with A-A cable I explained above, A-A cable should > >not be used in role-switching cases (it should be used at the first > >place), so this new property is unnecessary. > > > >> > >>>Using the sysfs entry to switch roles for generic purpose is really a > >>>bad idea, it opens up ton of problems. > >> > >>Yet the sysfs entry exists already, and the problems depend on how > >>you hook it up. I'm merely using the sysfs entry as an id-pin > >>for cases where there is no id pin hooked up. If you look at the > >>patches you will see that all that is changed by writing the > >>sysfs entry is the phy reporting a different id-pin value to > >>the musb ip. Everything else is handled the same as for any normal > >>otg mode port. > >> > >>>For systems which lack of id-pin should use a discrete circuit (for > >>>example GPIO) to detect the id pin in the AB receptacle, then the USB > >>>driver will handle the role switching transparently. > >> > >>You're again talking theory here in reality there is hardware where > >>for whatever reasons the PCB uses a mini or micro usb receptacle, > >>but they did not bother to _physically_ hook up the id pin. > > > >Who are they? > > One example of such a device is the quite popular mk802 tv stick > (the original version) with A10s SoC. > > > the manufactures who use musb controller to make products, > >so as Linux or musb drivers, we have control, it is our responsibility > >to tell those manufactures to do things in the right way - design the hw > >correctly at the first place. We can't compromise by using an A-A cable > >to provide a change to damage the usb host. > > > >This is different from the cases, for example, from the xhci controller > >perspective, the xhci driver has to compromise any usb thumb drives > >which do not follow the USB Specs, to make them work on PCs. > > That makes no sense, why would the xhci driver have to compromise > but we don't ? Either actually having things working is more important > then following the spec or the spec is more important... > > And in reality as your xhci example points out having things > working (and that means offering all possible functionality) is > more important. > > >>As said before reality trumps the Spec / theory. > >> > >>>>rather then peripheral mode which is the default for systems which lack an > >>>>id-pin. This patch set introduces: > >>>> > >>>>"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property" > >>>> > >>>>Which allows specifying the use of a female USB-A connector for the > >>>>musb controller in the phy dt node, the presence of this dt property > >>>>changes the default to host mode. > >>> > >>>This is unnecessary, if using a type-A connector, dr_mode should be > >>>"host" in DT. > >> > >>Which means we're telling users what they can and cannot do with > >>their hardware, which we should not be doing. > > > >To summary up my opinion from my lengthy comments above, > > > >A-A cable should not be used, especially in role-switching cases, so > >the DT prop is unnecessary; > > A<_>A cables exist, users will try to plug them in, those are facts. > > So we'd better be prepared to deal with this, denying this scenario > exists is not helpful, not supporting it is equally unhelpful. Well, I will stop arguing on this from here. It is up to the DT maintainer to decide on this DT prop. > > Regards, > > Hans Regards, -Bin. -- 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
