On Tue, 2009-07-14 at 14:30 +0200, Doyu Hiroshi (Nokia-D/Helsinki) wrote: > From: "Carmody Phil.2 (EXT-Ixonos/Helsinki)" <ext-phil.2.carmody@xxxxxxxxx> > Subject: RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else > Date: Tue, 14 Jul 2009 13:20:30 +0200 > > > On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote: > > > Phil, > > > > -----Original Message----- > > > > From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- > > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Phil Carmody > > > > Sent: Tuesday, July 14, 2009 6:03 AM > > > > > > > > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote: > > > > > Thanks for the patch, it only has indentation problems, this is the > > > > checkpatch output: > > > > > > > > > > WARNING: suspect code indent for conditional statements (8, 12) > > > > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152: > > > > > + if (DSP_SUCCEEDED(status)) { \ > > > > > + if (unlikely((src) == NULL) || \ > > > > > > > > > > WARNING: line over 80 characters > > > > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154: > > > > > + unlikely(copy_from_user(dest, src, (elements) * > > > > sizeof(*(dest))))) { \ > > > > > > > > > > WARNING: suspect code indent for conditional statements (8, 12) > > > > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164: > > > > > + if (DSP_SUCCEEDED(status)) { \ > > > > > + if (unlikely((dest) == NULL) || > > > > \ > > > > > > > > > > WARNING: line over 80 characters > > > > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166: > > > > > + unlikely(copy_to_user(dest, src, (elements) * > > > > sizeof(*(src))))) { \ > > > > > > > > > > total: 0 errors, 4 warnings, 48 lines checked > > > > > > > > > > Could you please fix that warning please? > > > > > > > > I suspect the only way to fix those warnings would either introduce > > > > other warnings, or \ > > > Gentle query: Have we already tried this or is it just a suspicion? > > > > Being an Englishman, I use various forms of irony more than others do; > > that particular form's called "litotes". It indeed is not a suspicion - > > I tried about 4 or 5 variations, none of which were both readable and > > warning-free. In the end I decided that whatever was closest to the > > original would be safest. > > > > > > would \ > > > > lead \ > > > > to \ > > > > utterly \ > > > > unread- \ > > > > able \ > > > > code. > > > > > > > > If you check how and why the original TI-originated version of the code > > > > does not follow the linux coding standards, the difficulties we would > > > > have making a warning-free patch of it should be apparent. > > > > > > Past is past. The idea was not to introduce anymore warning code. > > > > The TI code would trigger the following 4 warnings: > > WARNING: suspect code indent for conditional statements (4, 12) > > WARNING: line over 80 characters > > WARNING: suspect code indent for conditional statements (4, 12) > > WARNING: line over 80 characters > > 4 warnings isn't more than 4 warnings, it's no change - and as I mention > > above, in the absence of an easy fix, that was deliberate. > > I am not sure if these copy_from_user() wrapping are practically > useful or not. It may be enough just to use kernel API as is. But if > there are some reason to requires strict parameter checkings or > debugging feature support for these family here, introducing inline > functions for them, instead of macro, may be another option? I did mention that possibility to Ameya. In the 21st century, I think we should be able to trust the compiler to generate identical code from inline functions as functional macros. (Are we allowed to use 'restrict' pointers?) > For example, > > Modified drivers/dsp/bridge/pmgr/wcd.c > diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c > index 86812c6..c8b26c0 100644 > --- a/drivers/dsp/bridge/pmgr/wcd.c > +++ b/drivers/dsp/bridge/pmgr/wcd.c > @@ -146,16 +146,23 @@ > #define MAX_BUFS 64 > > /* Following two macros should ideally have do{}while(0) */ > +static inline void cp_fm_usr(void *to, const void __user *from, > + DSP_STATUS *err, unsigned long n) > +{ > + if (DSP_FAILED(err)) *err > + return; > > -#define cp_fm_usr(dest, src, status, elements) \ > - if (DSP_SUCCEEDED(status)) {\ > - if (unlikely(src == NULL) || \ > - unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \ > - GT_1trace(WCD_debugMask, GT_7CLASS, \ > - "copy_from_user failed, src=0x%x\n", src); \ > - status = DSP_EPOINTER ; \ > - } \ > - } > + if (unlikely(!from)) { > + *err = DSP_EPOINTER ; > + return; > + } > + > + if (unlikely(copy_from_user(to, from, n * sizeof(*(to))))) { additional () not needed any more. > + GT_1trace(WCD_debugMask, GT_7CLASS, > + "%s failed, from=0x%x\n", src, __func__); > + *err = DSP_EPOINTER ; > + } > +} > > #define cp_to_usr(dest, src, status, elements) \ > if (DSP_SUCCEEDED(status)) {\ > @@ -525,7 +532,7 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args) > char *pszPathName = NULL; > DSP_STATUS status = DSP_SOK; > > - cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, status, 1); > + cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, &status, 1); > if (DSP_FAILED(status)) > goto func_end; > pathSize = strlen_user((char *) > .... That kind of thing would work. There would be loads of changes (62, by the looks of it), but at least they are trivial ones. Phil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
