Date: Thu, 21 Aug 2008 00:18:35 +0200 From: Kris Kennaway <kris@FreeBSD.org> To: Alfred Perlstein <alfred@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] Message-ID: <48AC983B.4020409@FreeBSD.org> In-Reply-To: <20080820211644.GD16977@elvis.mu.org> References: <20080818205914.GJ16977@elvis.mu.org> <200808192219.19246.hselasky@c2i.net> <48AB3D3A.4040303@FreeBSD.org> <200808201809.24620.hselasky@c2i.net> <48AC54B8.4040406@FreeBSD.org> <20080820211644.GD16977@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Alfred Perlstein wrote: >>> Yes, and it would be nice if the sound maintainers would try out the new >>> USB stack, and propose how they want to solve the problems that exist in >>> the sound system. >> This is backwards. If you perceive problems in other subsystems you >> should take the lead on engaging with the relevant developers and >> resolving those problems to your mutual satisfaction, instead of >> inviting them to come to you. > > I think there has been a huge amount of effort just on the usb > side, I think expecting Hans to come up with a patch that > is "fix all freebsd issues" is a bit too much. This is a straw man argument, since no-one except you has mentioned "fixing all freebsd issues" in this thread about importing a new USB stack. The issue under discussion is a particular part of the patch that you and Hans are proposing to commit, which introduces changes to non-USB parts of the system that it turns out are completely unrelated to USB. Anyway, we now agree that this should not be part of the USB import, and I urge you or Hans to follow the normal FreeBSD process, which involves sending mail to the relevant developers. >>>>> There are multiple issues: >>>>> >>>>> 1) If PAGE_SIZE is 16384 bytes, then the code simply fails. >>>>> 2) Sometimes PAGE_SHIFT is already defined. >>>>> >>>>> #if PAGE_SIZE == 4096 >>>>> #define PAGE_SHIFT 12 >>>>> #elif PAGE_SIZE == 8192 >>>>> #define PAGE_SHIFT 13 >>>>> #else >>>>> #error PAGE_SHIFT undefined! >>>>> #endif >>>> OK, but again, why? If some architecture is not defining the same >>>> symbols as the others then maybe that architecture should be fixed, >>>> instead of working around the effect downstream. >>> Yes, but then there will be even more patches, and I've been told to >>> reduce the number of patches outside USB. >> The way you minimize patches outside of USB is by submitting separate >> patches that fix upstream issues at their source, instead of adding >> workarounds like several of those that appeared in this diff. > > It's a two line ifdef, I dare say we're being a bit pedantic here. > > How much of a showstopper is this in reality? How _wrong_ is it > compared to how _wrong_ the old usb code is? Are you taking into > account that these nits can be fixed easily compared to the extra > effort we will have to go through to submit all these stuff just for > a two line delta? Calling me pedantic is not an argument, nor is saying "but the old code is worse". If neither you nor Hans can explain why this part of the diff is the best solution to some incompletely specified problem, then it's likely not to be. What architecture is this change even relevant for? >>>> P.S. There were a number of questions you didn't answer, can I assume >>>> those will follow later? >>> Could you please repeat the questions you did not get an answer to? >> I've repeated some of them already. e.g. in this mail you still didn't >> answer the "is it safe to drop the locks" question which was asked >> twice. Rather than me repeating them yet again, I'd suggest you go back >> and take another close read over my emails and your responses, and reply >> to the questions that are still not resolved. > > Hans, let's just leave sound the way it is. I will remove that > part of the patch. It might be better to leave a bad locking > in that generates a warning rather than obscure the locking issue > by removing the warning without fully understanding the issues. OK. Based on this review thread I think we have agreed that: * The ARM changes were already imported separately by warner * the sound changes will not go in since they're unrelated to USB and there are some unresolved questions there. It sounds like there are real bugs identified and possibly also fixed there, so I continue to encourage you or Hans to follow up with the sound developers so the problems can be resolved separately. * the newbus change is not needed (but warner liked Hans's helper function so maybe that should be committed separately) * the ndis module is not complete and will not go in yet, and the question about the above #ifdef will be resolved in the meantime * hans is going to work on adding back the individual drivers instead of bundling them * you and hans will work on the manpages post-commit with help from the doc guys Questions remaining: * Why is the PAGE_SHIFT change necessary? * The $FreeBSD$ expansion in files that were copied - will this confuse SVN or the CVS exporter? In CVS we had to make sure not to import expanded tags. Easiest solution is to just unexpand them. * How much of the userland support is incomplete or in flux, and what functionality is missing for users of the usb2 code until it is finished? Kris
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48AC983B.4020409>