Hi David, >>>>> This is my second attempt to get these patches merged. I added 4 more fixes at >>>>> the end and removed the patches that were already merged by Gustavo. >>>>> >>>>> This fixes a lot of issues with the HIDP layer. It is currently nearly >>>>> impossible to use HIDP devices. Every attempt (except the first) to connect a >>>>> device results in an oops or panic for me. >>>>> >>>>> I haven't found a way to fix this easily, so I decided to rewrite the session >>>>> handling of HIDP. >>>> >>>> I am generally fine with these patches after a quick read-through. However I am not 100% sure that hci_conn_user is the right approach. >>>> >>>> So my question is if we really want to base this of the ACL link itself and not base this around L2CAP directly. Every L2CAP connection implies a hci_conn in the end. Would it be hard to have something like l2cap_user instead of hci_conn_user? >>> >>> The thing is that HID uses two L2CAP channels. We would probably use >>> the CTRL channel as parent device, but this makes the synchronization >>> with the INTR channel quite hard to implement. >>> Also note that l2cap channels do not have "struct device" embedded, >>> which would disallow showing the HID device in the right location in >>> sysfs. >>> If you have some other idea, I can try to implement this. But I >>> couldn't come up with anything better than hid_conn_user. >> >> there is a difference between l2cap_conn and l2cap_chan. The L2CAP layer has a generic representation of underlying ACL link. I was talking about that one. From that we can get to the hci_conn device object. >> >> Maybe this is more cosmetic, but only going through l2cap_conn and have that bridge into hci_conn seems a bit cleaner to me. Especially since you need a bunch of exports from L2CAP layer anyway. > > Sorry, I thought of l2cap_chan.. Sure, l2cap_conn actually makes > sense. I already use a helper to get the hci_conn out of l2cap_sock so > I might just as easily try to get l2cap_conn instead. > > But this is indeed only a cosmetic change, because l2cap_conn->hconn > is always valid so I will just move it over to l2cap_conn_user. > I will resend in a few days, so if there are something else, I can fix it, too. make it l2cap_user to keep it smaller. I really dislike super long names. And if it does not work, then we go with the hci_conn_user stuff. > Btw., I tested this with 5 simultaneously connected devices, I tested > device reconnects, I tested system suspend, ... and everything works > fine here. My comments where more cosmetic since exposing HCI all the way up to HIDP seems a bit bad. If we can hide it behind L2CAP, then it looks a lot better. The code itself and the changes looked all fine to me. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
