Hi Leif, Neta,
2016-08-19 14:48 GMT+02:00 Leif Lindholm leif.lindholm@linaro.org:
On Fri, Aug 19, 2016 at 01:23:31PM +0200, Ard Biesheuvel wrote:
At which point I'm quite OK with the "importing", as long as it's wrapped into something that does not require components accessing it to have special knowledge of its internal typedefs and macros.
But I'd also be astonished if David Miller accepted the MV_UINT32 type defines.
That's more than certain, as well as typedefs, so _if_ this was submitted (IMO not likely), for sure it wouldn't end up in the shape as we have now, i.e. completely OS-independent code with a glue.
If there isn't a common upstream for this driver between projects, then they will diverge and whichever ends up with more devs will be the source of patches and hence their code style will be de facto standard. Plus we won't be able to merge GPL licensed contributions anyway, so we can't really be a downstream of the Linux driver.
Since Marvell is the owner of the code, I'd prefer to ask first if there are any objections to change the style and leave single BSD licence. I would just make sure, the library remains fully OS-independent.
Neta, what do you think you can accept above?
So in that case, yes, I would prefer EDK2-ising it. But I'd appreciate Ard's input. Either way, we should start by isolating the typedefs and macros into a separate header forming part of 1/3.
I think we should be pragmatic about importing existing code. I care very little about things like STATIC, VOID, and CONST, since they are unconditionally #defined to static, void and const, respectively, anyway.
Sure, but that's the lesser problem here.
Also, checkpatch scripts are useful for spotting changes that result in coding style violations in files that were formerly compliant. Taking existing code and then perform blanket search/replace on them for things like the above is a waste of effort imo, especially if there are other changes required that may actually affect code size or correctness in UEFI context, like alignment or struct assignment, things that are forbidden for a reason. If such changes are needed, I would like to see them in a separate patch that goes on top of the original introduction of the new code.
Right, but I'm not obsessing over what the initial commit should look like, I'm obsessing over what the resulting code should look like.
We cannot have the consumers of said imported patches needing to redefine a bunch of datatypes in order to call them. But also, if we are not expecting to be seeing some sort of destination-independent updates to this driver from the vendor, I see little value in keeping the code non-compliant.
Frankly, if a rework could be done, I a LOT prefer to do it now, rather than it unexpectedly hit us when you merge OPP into EDK2.
About tracking fixes and modifications in the other sources - it would be great, but I wouldn't bother that much. Basically, the bootloader's network is needed mostly for downloading files. Current state of Pp2Dxe is that in different phy modes (SGMII, RGMII, fixed link switch) it works stable, we can tftp 500MB file within 1 minute - so it's in a satisfying state. Performance @highest rates should not be a question here. After all, possible modifications will have to be applies manually and if they refer to particular patch somewhere else, I think caring about mentioning the source in the commit log should be sufficient.
In order to summary, I propose following view of v2 patchset: 1. Import library as-is now (except for licence, which has to fit uefi from the beginning). 2. Add style modification to pass edk2 checkpatch. 3. Add separate glue header. 4. Add driver 5. Enable driver. Pairs 1-2 and 3-4 can be squashed, but we can discuss it later.
What do you think?
Best regards, Marcin