| Message ID | 20210514200453.1542978-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/surface: aggregator: avoid clang -Wconstant-conversion warning | expand |
On 5/14/2021 1:04 PM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Clang complains about the assignment of SSAM_ANY_IID to > ssam_device_uid->instance: > > drivers/platform/surface/surface_aggregator_registry.c:478:25: error: implicit conversion from 'int' to '__u8' (aka 'unsigned char') changes value from 65535 to 255 [-Werror,-Wconstant-conversion] > { SSAM_VDEV(HUB, 0x02, SSAM_ANY_IID, 0x00) }, > ~ ^~~~~~~~~~~~ > include/linux/surface_aggregator/device.h:71:23: note: expanded from macro 'SSAM_ANY_IID' > #define SSAM_ANY_IID 0xffff > ^~~~~~ > include/linux/surface_aggregator/device.h:126:63: note: expanded from macro 'SSAM_VDEV' > SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun) > ^~~ > include/linux/surface_aggregator/device.h:102:41: note: expanded from macro 'SSAM_DEVICE' > .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > ^~~ > > The assignment doesn't actually happen, but clang checks the type limits > before checking whether this assignment is reached. Replace the ?: > operator with a __builtin_choose_expr() invocation that avoids the > warning for the untaken part. > > Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type") > Cc: platform-driver-x86@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > v2: use __builtin_choose_expr() instead of a cast to shut up the warning > --- > include/linux/surface_aggregator/device.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h > index 4441ad667c3f..6ff9c58b3e17 100644 > --- a/include/linux/surface_aggregator/device.h > +++ b/include/linux/surface_aggregator/device.h > @@ -98,9 +98,9 @@ struct ssam_device_uid { > | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \ > .domain = d, \ > .category = cat, \ > - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ > - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ > + .target = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0), \ > + .instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0), \ > + .function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0) > > /** > * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with >
On 14/05/2021 22:04, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Clang complains about the assignment of SSAM_ANY_IID to > ssam_device_uid->instance: > > drivers/platform/surface/surface_aggregator_registry.c:478:25: error: implicit conversion from 'int' to '__u8' (aka 'unsigned char') changes value from 65535 to 255 [-Werror,-Wconstant-conversion] > { SSAM_VDEV(HUB, 0x02, SSAM_ANY_IID, 0x00) }, > ~ ^~~~~~~~~~~~ > include/linux/surface_aggregator/device.h:71:23: note: expanded from macro 'SSAM_ANY_IID' > #define SSAM_ANY_IID 0xffff > ^~~~~~ > include/linux/surface_aggregator/device.h:126:63: note: expanded from macro 'SSAM_VDEV' > SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun) > ^~~ > include/linux/surface_aggregator/device.h:102:41: note: expanded from macro 'SSAM_DEVICE' > .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > ^~~ > > The assignment doesn't actually happen, but clang checks the type limits > before checking whether this assignment is reached. Replace the ?: > operator with a __builtin_choose_expr() invocation that avoids the > warning for the untaken part. > > Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type") > Cc: platform-driver-x86@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thanks! This looks good to me. Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > v2: use __builtin_choose_expr() instead of a cast to shut up the warning > --- > include/linux/surface_aggregator/device.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h > index 4441ad667c3f..6ff9c58b3e17 100644 > --- a/include/linux/surface_aggregator/device.h > +++ b/include/linux/surface_aggregator/device.h > @@ -98,9 +98,9 @@ struct ssam_device_uid { > | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \ > .domain = d, \ > .category = cat, \ > - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ > - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ > + .target = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0), \ > + .instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0), \ > + .function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0) > > /** > * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with >
From: Nathan Chancellor > Sent: 14 May 2021 22:23 > > > > Clang complains about the assignment of SSAM_ANY_IID to > > ssam_device_uid->instance: Has this been raised with clang? ... > > - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ > > - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > > - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ > > + .target = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0), \ > > + .instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0), \ > > + .function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0) A simpler alternative: = fun * (fun != SSAM_ANY_FUN) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 5/17/21 10:31 AM, David Laight wrote: > From: Nathan Chancellor >> Sent: 14 May 2021 22:23 >>> >>> Clang complains about the assignment of SSAM_ANY_IID to >>> ssam_device_uid->instance: > > Has this been raised with clang? I believe so, see this earlier report: https://lore.kernel.org/linux-mm/20210311185154.6uysryumfho73zlb@archlinux-ax161/ Regards, Max
Hi, On 5/14/21 10:04 PM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Clang complains about the assignment of SSAM_ANY_IID to > ssam_device_uid->instance: > > drivers/platform/surface/surface_aggregator_registry.c:478:25: error: implicit conversion from 'int' to '__u8' (aka 'unsigned char') changes value from 65535 to 255 [-Werror,-Wconstant-conversion] > { SSAM_VDEV(HUB, 0x02, SSAM_ANY_IID, 0x00) }, > ~ ^~~~~~~~~~~~ > include/linux/surface_aggregator/device.h:71:23: note: expanded from macro 'SSAM_ANY_IID' > #define SSAM_ANY_IID 0xffff > ^~~~~~ > include/linux/surface_aggregator/device.h:126:63: note: expanded from macro 'SSAM_VDEV' > SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun) > ^~~ > include/linux/surface_aggregator/device.h:102:41: note: expanded from macro 'SSAM_DEVICE' > .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > ^~~ > > The assignment doesn't actually happen, but clang checks the type limits > before checking whether this assignment is reached. Replace the ?: > operator with a __builtin_choose_expr() invocation that avoids the > warning for the untaken part. > > Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type") > Cc: platform-driver-x86@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I will also include this in the next pdx86-fixes pull-req for 5.13. Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > v2: use __builtin_choose_expr() instead of a cast to shut up the warning > --- > include/linux/surface_aggregator/device.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h > index 4441ad667c3f..6ff9c58b3e17 100644 > --- a/include/linux/surface_aggregator/device.h > +++ b/include/linux/surface_aggregator/device.h > @@ -98,9 +98,9 @@ struct ssam_device_uid { > | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \ > .domain = d, \ > .category = cat, \ > - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ > - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ > - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ > + .target = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0), \ > + .instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0), \ > + .function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0) > > /** > * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with >
diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h index 4441ad667c3f..6ff9c58b3e17 100644 --- a/include/linux/surface_aggregator/device.h +++ b/include/linux/surface_aggregator/device.h @@ -98,9 +98,9 @@ struct ssam_device_uid { | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \ .domain = d, \ .category = cat, \ - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ + .target = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0), \ + .instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0), \ + .function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0) /** * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with