Hi Takashi, Thanks for your comments. I addressed the style issues and I resubmitted the patches (V3). Note about the MMX patch: The mm1 register is read by the chunk, but its value is not used (the instruction packssdw put the value of mm1 in the high 32 bits of mm0 but then, only the low 32 bits are used. My tool made an over-approximation but it is now fixed). By the way, the first 3 patches produce the same binary output as master. However, I looked in the 'test' folder, but I do not know how to run a test for the pcm. For now, I have no time to investigate how alsa should be run till the next week. > But there were some issues with the patch format. IIRC, the > patch 2 couldn't be applied to the latest git tree cleanly (some space > etter issues?), so I had to manually modify it. Sorry for that, it looks like my text editor remove space at the end of the line automatically. It should not be the case with the new patches. Regards, Frédéric De: "tiwai" <tiwai@xxxxxxx> À: "FRÉDÉRIC RECOULES" <frederic.recoules@xxxxxxxxxxxxxxxxxxxxxx> Cc: "alsa-devel" <alsa-devel@xxxxxxxxxxxxxxxx>, "frederic recoules" <frederic.recoules@xxxxxxxxx> Envoyé: Mardi 5 Mai 2020 15:52:19 Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces On Mon, 04 May 2020 21:25:16 +0200, FRÉDÉRIC RECOULES wrote: > > Hi Takashi, > > I would like an update on the review process ([PATCH */6 V2] [pcm_dmix > assembly]) > > As a reminder, I split the changes in 6 distinct patches. > The 3 first patches produce exactly the same binary output, so they do not > need testing. > The 4th one has just some minor change due to the fact that I added an > instruction -- I am highly confident it breaks nothing. The compile tests passed with a few different compiler set, so that's good. But there were some issues with the patch format. IIRC, the patch 2 couldn't be applied to the latest git tree cleanly (some space letter issues?), so I had to manually modify it. Also, some style issues: - Please avoid a prefix like "[configure]" in the subject. The prefix with "[PATCH xxx]" is good, and this should remain, but the next prefix should be "configure:" instead. Otherwise the prefix with the brackets are pruned at applying a patch via git-am. - Please give more texts about why the change is done. In all your patches, there are no explanations why you change it. It's often more important than describing what you're changing. For example, the patch 2 "change the token by symbolic names". Why is this needed to be symbolic names? Write some more information in each patch description. - We usually use #ifdef without space between "#" and "ifdef". Let's keep that style consistently. > If you need I test the 2 last ones (that reduce the size of the produced > binary), could you point me out what test I should run? We need at least some build tests with different compiler versions and check whether dmix actually works (not necessarily on all of them but some of those compiled results). > Meanwhile, my deadline comes and I would really appreciate to see the patches > applied by wednesday night. If you can work on the above and resubmit v3 patchset, I'll happily apply them. Thanks! Takashi
