On Mon, Oct 12, 2020 at 07:07:55AM -0700, Tom Rix wrote:
>
> On 10/11/20 7:41 PM, Xu Yilun wrote:
> > On Sat, Oct 10, 2020 at 08:07:07AM -0700, Tom Rix wrote:
> >> On 10/10/20 12:09 AM, Xu Yilun wrote:
> >>> The value of the field dfl_device.type comes from the 12 bits register
> >>> field DFH_ID according to DFL spec. So this patch changes the definition
> >>> of the type field to u16.
> >>>
> >>> Also it is not necessary to illustrate the valid bits of the type field
> >>> in comments. Instead we should explicitly define the possible values in
> >>> the enumeration type for it, because they are shared by hardware spec.
> >>> We should not let the compiler decide these values.
> >>>
> >>> Similar changes are also applied to dfl_device.feature_id.
> >>>
> >>> This patch also fixed the MODALIAS format according to the changes
> >>> above.
> >>>
> >>> Signed-off-by: Xu Yilun <yilun...@intel.com>
> >>> ---
> >>> v9: no change
> >>> ---
> >>> drivers/fpga/dfl.c | 3 +--
> >>> drivers/fpga/dfl.h | 14 +++++++-------
> >>> 2 files changed, 8 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> >>> index b450870..5a6ba3b 100644
> >>> --- a/drivers/fpga/dfl.c
> >>> +++ b/drivers/fpga/dfl.c
> >>> @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct
> >>> kobj_uevent_env *env)
> >>> {
> >>> struct dfl_device *ddev = to_dfl_dev(dev);
> >>>
> >>> - /* The type has 4 valid bits and feature_id has 12 valid bits */
> >>> - return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> >>> + return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
> >>> ddev->type, ddev->feature_id);
> >>> }
> >>>
> >>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> >>> index 5dc758f..ac373b1 100644
> >>> --- a/drivers/fpga/dfl.h
> >>> +++ b/drivers/fpga/dfl.h
> >>> @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct
> >>> platform_device *pdev,
> >>> * enum dfl_id_type - define the DFL FIU types
> >>> */
> >>> enum dfl_id_type {
> >>> - FME_ID,
> >>> - PORT_ID,
> >>> + FME_ID = 0,
> >>> + PORT_ID = 1,
> >> This is redundant, why make this change ?
> > These values are shared by hardware spec, so it is suggested that the
> > values of the enum type should be explicitly set, otherwise the compiler
> > is in its right to do whatever it wants with them (within reason...)
> >
> > Please see the original discussion:
> > https://lore.kernel.org/linux-fpga/20200923055436.ga2629...@kroah.com/
>
> I don't believe this is undefined behavior for the compiler
>
> from c11 6.7.2.2,3
>
> The identifiers in an enumerator list are declared as constants that have
> type int and may appear wherever such are permitted.127) An enumerator with =
> defines its enumeration constant as the value of the constant expression. If
> the first enumerator has no =, the value of its enumeration constant is 0.
> Each subsequent enumerator with no = defines its enumeration constant as the
> value of the constant expression obtained by adding 1 to the value of the
> previous enumeration constant. (The use of enumerators with = may produce
> enumeration constants with values that duplicate other values in the same
> enumeration.) The enumerators of an enumeration are also known as its members.
>
> setting them again has some use for documentation so this change is ok if you
> have strong feeling for it.
The kernel developer community has "strong feelings" for this, please be
specific and list the values when they matter.
thanks,
greg k-h
