lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB3819B9B2F357B9B03F7707B685099@DM6PR11MB3819.namprd11.prod.outlook.com>
Date:   Tue, 22 Jun 2021 07:39:05 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     "Xu, Yilun" <yilun.xu@...el.com>
CC:     Martin Hundebøll <mhu@...icom.dk>,
        Tom Rix <trix@...hat.com>, Moritz Fischer <mdf@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Lee Jones <lee.jones@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Martin Hundebøll <mhu@...nix.com>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        Debarati Biswas <debaratix.biswas@...el.com>,
        "Weight, Russell H" <russell.h.weight@...el.com>
Subject: RE: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
  linux/dfl.h

> On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
> > > Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
> linux/dfl.h
> > >
> > > From: Debarati Biswas <debaratix.biswas@...el.com>
> > >
> > > Device Feature List (DFL) drivers may be defined in subdirectories other
> > > than drivers/fpga, and each DFL driver should have access to the Device
> > > Feature Header (DFH) register, which contains revision and type
> > > information. This change moves the macros specific to the DFH register
> > > from drivers/fpga/dfl.h to include/linux/dfl.h.
> >
> > Looks like it requires to access the revision info in the next patch, because
> > current dfl_device doesn't expose related information.
> >
> > @Yilun, do you have any concern to expose those info via dfl_device?
> 
> Exposing these header register definitions are good to me. These registers
> are in DFL device's MMIO region, so it is good to share these info with
> all DFL drivers.

I mean expose revision via dfl_device, as dfl core already reads the DFL
header, it sounds duplicate read in each dfl device driver. And if we
consider this as a common need from dfl device driver, then the code
can be moved to a common place as well.

I hope from dfl device driver side, it doesn't need to know details of
how DFH register is defined, only simple way from dfl device data
structure or some simple helper function, then dfl device driver could
know all common information from DFH.

How do you think?

Thanks
Hao

> 
> Thanks,
> Yilun
> 
> >
> > Thanks
> > Hao
> >
> > >
> > > Signed-off-by: Debarati Biswas <debaratix.biswas@...el.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> > > Signed-off-by: Martin Hundebøll <mhu@...icom.dk>
> > > ---
> > >  drivers/fpga/dfl.h  | 48 +----------------------------------------
> > >  include/linux/dfl.h | 52
> +++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 53 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > > index 2b82c96ba56c..6ed0353e9a99 100644
> > > --- a/drivers/fpga/dfl.h
> > > +++ b/drivers/fpga/dfl.h
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/bitfield.h>
> > >  #include <linux/cdev.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/dfl.h>
> > >  #include <linux/eventfd.h>
> > >  #include <linux/fs.h>
> > >  #include <linux/interrupt.h>
> > > @@ -53,32 +54,6 @@
> > >  #define PORT_FEATURE_ID_UINT         0x12
> > >  #define PORT_FEATURE_ID_STP          0x13
> > >
> > > -/*
> > > - * Device Feature Header Register Set
> > > - *
> > > - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header
> registers.
> > > - * For AFUs, they have DFH + GUID as common header registers.
> > > - * For private features, they only have DFH register as common header.
> > > - */
> > > -#define DFH                  0x0
> > > -#define GUID_L                       0x8
> > > -#define GUID_H                       0x10
> > > -#define NEXT_AFU             0x18
> > > -
> > > -#define DFH_SIZE             0x8
> > > -
> > > -/* Device Feature Header Register Bitfield */
> > > -#define DFH_ID                       GENMASK_ULL(11, 0)      /* Feature ID
> > > */
> > > -#define DFH_ID_FIU_FME               0
> > > -#define DFH_ID_FIU_PORT              1
> > > -#define DFH_REVISION         GENMASK_ULL(15, 12)     /* Feature revision */
> > > -#define DFH_NEXT_HDR_OFST    GENMASK_ULL(39, 16)     /* Offset to next
> DFH
> > > */
> > > -#define DFH_EOL                      BIT_ULL(40)             /* End of list
> > > */
> > > -#define DFH_TYPE             GENMASK_ULL(63, 60)     /* Feature type */
> > > -#define DFH_TYPE_AFU         1
> > > -#define DFH_TYPE_PRIVATE     3
> > > -#define DFH_TYPE_FIU         4
> > > -
> > >  /* Next AFU Register Bitfield */
> > >  #define NEXT_AFU_NEXT_DFH_OFST       GENMASK_ULL(23, 0)      /* Offset
> to
> > > next AFU */
> > >
> > > @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct
> > > dfl_feature_platform_data *pdata)
> > >       return pdata->dev->dev.parent->parent;
> > >  }
> > >
> > > -static inline bool dfl_feature_is_fme(void __iomem *base)
> > > -{
> > > -     u64 v = readq(base + DFH);
> > > -
> > > -     return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > -             (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > -}
> > > -
> > > -static inline bool dfl_feature_is_port(void __iomem *base)
> > > -{
> > > -     u64 v = readq(base + DFH);
> > > -
> > > -     return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > -             (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > -}
> > > -
> > > -static inline u8 dfl_feature_revision(void __iomem *base)
> > > -{
> > > -     return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > -}
> > > -
> > >  /**
> > >   * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> > >   *
> > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > index 6cc10982351a..1cd86b2e7cb1 100644
> > > --- a/include/linux/dfl.h
> > > +++ b/include/linux/dfl.h
> > > @@ -8,7 +8,9 @@
> > >  #ifndef __LINUX_DFL_H
> > >  #define __LINUX_DFL_H
> > >
> > > +#include <linux/bitfield.h>
> > >  #include <linux/device.h>
> > > +#include <linux/io.h>
> > >  #include <linux/mod_devicetable.h>
> 
> The <linux/io-64-nonatomic-lo-hi.h> is needed here, or readq/writeq
> definitions may be missing on some platform, as the kernel test robot
> says.
> 
> Thanks,
> Yilun
> 
> > >
> > >  /**
> > > @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > >       module_driver(__dfl_driver, dfl_driver_register, \
> > >                     dfl_driver_unregister)
> > >
> > > +/*
> > > + * Device Feature Header Register Set
> > > + *
> > > + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header
> registers.
> > > + * For AFUs, they have DFH + GUID as common header registers.
> > > + * For private features, they only have DFH register as common header.
> > > + */
> > > +#define DFH                     0x0
> > > +#define GUID_L                  0x8
> > > +#define GUID_H                  0x10
> > > +#define NEXT_AFU                0x18
> > > +
> > > +#define DFH_SIZE                0x8
> > > +
> > > +/* Device Feature Header Register Bitfield */
> > > +#define DFH_ID                  GENMASK_ULL(11, 0)      /* Feature ID */
> > > +#define DFH_ID_FIU_FME          0
> > > +#define DFH_ID_FIU_PORT         1
> > > +#define DFH_REVISION            GENMASK_ULL(15, 12)
> > > +#define DFH_NEXT_HDR_OFST       GENMASK_ULL(39, 16)     /* Offset to
> next
> > > DFH */
> > > +#define DFH_EOL                 BIT_ULL(40)             /* End of list */
> > > +#define DFH_TYPE                GENMASK_ULL(63, 60)     /* Feature type */
> > > +#define DFH_TYPE_AFU            1
> > > +#define DFH_TYPE_PRIVATE        3
> > > +#define DFH_TYPE_FIU            4
> > > +
> > > +/* Function to read from DFH and check if the Feature type is FME */
> > > +static inline bool dfl_feature_is_fme(void __iomem *base)
> > > +{
> > > +     u64 v = readq(base + DFH);
> > > +
> > > +     return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > +             (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > +}
> > > +
> > > +/* Function to read from DFH and check if the Feature type is port*/
> > > +static inline bool dfl_feature_is_port(void __iomem *base)
> > > +{
> > > +     u64 v = readq(base + DFH);
> > > +
> > > +     return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > +              (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > +}
> > > +
> > > +/* Function to read feature revision from DFH */
> > > +static inline u8 dfl_feature_revision(void __iomem *base)
> > > +{
> > > +     return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > +}
> > > +
> > >  #endif /* __LINUX_DFL_H */
> > > --
> > > 2.31.0
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ