| 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
| ||
|
Message-ID: <20170502115308.1e2f1233@crub>
Date: Tue, 2 May 2017 11:53:08 +0200
From: Anatolij Gustschin <agust@...x.de>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linux-fpga@...r.kernel.org,
Alan Tull <atull@...nsource.altera.com>,
Moritz Fischer <moritz.fischer@...us.com>,
matthew.gerlach@...ux.intel.com, yi1.li@...ux.intel.com,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] fpga manager: Add Altera CvP driver
On Mon, 1 May 2017 23:06:16 +0300
Andy Shevchenko andy.shevchenko@...il.com wrote:
>On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin <agust@...x.de> wrote:
>> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>> and Arria-10 FPGAs via CvP.
>
>I think you need to spend time on polishing such code.
>
>See my comments below.
>
>> +#define CVP_BAR 0 /* BAR used for data transfer in memory mode */
>> +#define CVP_DUMMY_WR 244 /* dummy writes to clear CvP state machine */
>> +#define TIMEOUT_IN_US 2000
>> +
>> +/* Vendor Specific Extended Capability Offset */
>> +#define VSEC_OFFSET 0x200
>
>> +#define VSEC_PCIE_EXT_CAP_ID_VAL 0x000b
>
>> +#define VSEC_PCIE_EXT_CAP_ID (VSEC_OFFSET + 0x00) /* 16bit */
>
>It is not clear what is what. If the above is value, why it's located
>under offset "section"?
ok, will move VSEC_PCIE_EXT_CAP_ID_VAL below the offset.
>> +#define VSEC_CVP_STATUS (VSEC_OFFSET + 0x1c) /* 32bit */
>
>Usually in the drivers we use just absolute value.
ok, will change it.
>> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20) /* 32bit */
>> +#define VSEC_CVP_MODE_CTRL_CVP_MODE BIT(0) /* CVP (1) or normal mode (0) */
>> +#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL BIT(1) /* PMA (1) or fabric clock (0) */
>> +#define VSEC_CVP_MODE_CTRL_FULL_CFG BIT(2) /* CVP_FULLCONFIG */
>
>> +#define VSEC_CVP_MODE_CTRL_NUMCLKS (0xff<<8) /* CVP_NUMCLKS */
>
>Is 0xff a mask here? (Btw, you missed spaces around <<)
yes, it is. Will add spaces (checkpatch didn't warn here).
>> +static int chkcfg; /* use value 1 for debugging only */
>> +module_param(chkcfg, int, 0664);
>> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 0)");
>> +
>> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
>> +module_param(numclks, int, 0664);
>> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");
>
>Why do we need that?!
>In new drivers we try to avoid new module parameters. We have enough
>interfaces nowadays to let driver know some details (quirks).
Which interface is preffered here? Do you suggest sysfs? Won't be able
to pass the parameter on kernel command line, then.
I'll drop the numclks parameter here and will use a fixed value. I do not
need it for current configurations and if anyone needs it and actually has
the devices and bitstreams to test it, feel free to implement it using the
preferred interfaces.
>> +
>> +struct altera_cvp_conf {
>> + struct fpga_manager *mgr;
>> + struct pci_dev *pci_dev;
>> + void __iomem *map;
>> + void (*write_data)(struct altera_cvp_conf *conf,
>> + u32 val);
>> + char mgr_name[64];
>> +};
>> +
>
>> +static inline void altera_cvp_chk_numclks(void)
>> +{
>> + switch (numclks) {
>> + case 1:
>> + case 4:
>> + case 8:
>> + break;
>> + default:
>> + numclks = 1;
>> + break;
>> + }
>> +}
>
>Why 2 is missed? Hardware limitation? Needs a comment about these magics.
it is not missed, other values are just not valid and filtered out here.
>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> + u32 val32;
>
>> + int i;
>
>unsigned int i; ?
ok, will change.
>> + /* set number of CVP clock cycles for every CVP Data Register Write */
>> + pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, &val32);
>> + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
>
>> + val32 |= 0x01 << 8; /* 1 clock */
>
>Yeah, needs more clear way to put clocks of choice.
what exactly is not clear here?
>> + pci_write_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> + for (i = 0; i < CVP_DUMMY_WR; i++)
>
>> + conf->write_data(conf, 0xdeadbeef);
>
>Why this dummy is chosen?
it is a dummy and can be anything. So why not? I re-used some code
where this value was chosen. Can change it to 0.
>> +}
>
>> +
>> +static int altera_cvp_teardown(struct fpga_manager *mgr,
>> + struct fpga_image_info *info)
>> +{
>> + struct altera_cvp_conf *conf = mgr->priv;
>> + struct pci_dev *pdev = conf->pci_dev;
>> + int status = 0;
>> + int delay_us;
>> + u32 val32;
>> +
>
>> + /*
>> + * STEP 12 - reset START_XFER bit
>> + */
>
>One line?
>(as well below)
ok, will change.
>> + pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>> + val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
>> + pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>> +
>
>> + delay_us = 0;
>> + while (1) {
>
>Oh, no. It's a red flag.
>
>And better to do
>
>do {} while (--retries);
can change it if its preferred.
>
>style. It will on smallest glance give reader a clue that the body
>will go at least once.
>
>> + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
>> + if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>> + break;
>> +
>
>> + udelay(1); /* wait 1us */
>
>Why not 10? Needs a comment.
if this is not obvious, we want to start the configuration early and want
to avoid unneeded delays when polling ready status. For 10 I would have
to use usleep_range() adding more delay.
>
>> +
>> + if (delay_us++ > TIMEOUT_IN_US) {
>> + dev_warn(&mgr->dev, "CVP_CONFIG_READY == 0 timeout\n");
>> + status = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> +
>> + return status;
>> +}
>
>> +
>> +static int altera_cvp_write_init(struct fpga_manager *mgr,
>> + struct fpga_image_info *info,
>> + const char *buf, size_t count)
>> +{
>> + struct altera_cvp_conf *conf = mgr->priv;
>> + struct pci_dev *pdev = conf->pci_dev;
>> + int delay_us;
>> + u32 val32;
>> + int ret;
>
>> + /*
>> + * STEP 1 - read CVP status and check CVP_EN flag
>> + */
>
>Ditto about comments.
>
>> + delay_us = 0;
>> + while (1) {
>
>Ditto about loop style.
ok, will change.
>> + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
>> + if ((val32 & VSEC_CVP_STATUS_CFG_RDY) ==
>> + VSEC_CVP_STATUS_CFG_RDY)
>> + break;
>> +
>> + udelay(1); /* wait 1us */
>> +
>> + if (delay_us++ > TIMEOUT_IN_US) {
>> + dev_warn(&mgr->dev, "CVP_CONFIG_READY == 1 timeout\n");
>> + return -ETIMEDOUT;
>> + }
>> + }
>
>> + return 0;
>> +}
>
>> +
>> +static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
>> +{
>> + struct altera_cvp_conf *conf = mgr->priv;
>> + u32 val32;
>
>> + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
>> + if (val32 & VSEC_CVP_STATUS_CFG_ERR) {
>
>> + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zi bytes!\n",
>> + bytes);
>
>%zu?
ok, will fix.
>> + return -EPROTO;
>> + }
>> + return 0;
>> +}
>
>> +
>> +static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>> + size_t count)
>> +{
>> + struct altera_cvp_conf *conf = mgr->priv;
>> + const u32 *data;
>> + size_t bytes;
>> + int status = 0;
>> + u32 mask;
>> +
>
>> + data = (u32 *)buf;
>> + bytes = count;
>> +
>> + while (bytes >= 4) {
>> + conf->write_data(conf, *data++);
>> + bytes -= 4;
>> +
>> + /*
>> + * STEP 10 (optional) and STEP 11
>> + * - check error flag
>> + * - loop until data transfer completed
>> + */
>
>> + if (chkcfg && !(bytes % SZ_4K)) {
>
>Is 4k comes from PCI spec, or is it page size?
no, it is more an arbitrary value. It was suggested to check for
error status after writing a data block and not after each data write
to speed-up the config process. The config images can be big (above
36 MiB) and often checking will slow down the configuration.
>> + size_t done = count - bytes;
>> +
>> + status = altera_cvp_chk_error(mgr, done);
>> + if (status < 0)
>> + return status;
>> + }
>> + }
>> +
>
>> + switch (bytes) {
>> + case 3:
>> + mask = 0x00ffffff;
>> + break;
>> + case 2:
>> + mask = 0x0000ffff;
>> + break;
>> + case 1:
>> + mask = 0x000000ff;
>> + break;
>> + case 0:
>> + mask = 0x00000000;
>> + break;
>> + }
>
>Why not to use simple formula?
>
>mask = BIT(bytes * 8) - 1;
ok, will use it.
>> +
>> + if (mask) {
>> + conf->write_data(conf, *data & mask);
>> +
>> + if (chkcfg)
>> + status = altera_cvp_chk_error(mgr, count);
>> + }
>> +
>> + return status;
>> +}
>
>> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
>> + struct fpga_image_info *info)
>> +{
>
>> + delay_us = 0;
>> + status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE;
>> + while (1) {
>
>Same comment about loop style.
ok, will change.
>> + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
>> + if ((val32 & status_msk) == status_msk)
>> + break;
>> +
>> + udelay(1); /* wait 1us */
>> +
>> + if (delay_us++ > TIMEOUT_IN_US) {
>> + dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
>> + status = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> +
>> + return status;
>> +}
>
>> +static int altera_cvp_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *dev_id)
>> +{
>> + struct altera_cvp_conf *conf;
>> + u16 cmd, val16;
>> + int ret;
>> +
>> + pci_read_config_word(pdev, VSEC_PCIE_EXT_CAP_ID, &val16);
>
>Are you foing to do this without enabling device? Needs comment why if so.
pci config space access works without enabling the pci device,
writing commands to config space enables the device first. It is done
some lines below which you deleted when commenting (please see original
patch).
>> + if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
>> + dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
>> + ret = -ENODEV;
>> + goto err;
>> + }
>> +
>
>> + conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> + if (!conf) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + conf->pci_dev = pdev;
>> +
>
>> + if (pci_request_region(pdev, CVP_BAR, "CVP") < 0) {
>
>So, you are using devm_ above, but avoid pcim_ here. Please clarify
>enabling device case and use if possible pcim_
I can't use pcim_enable_device(), it will make the driver unusable
on some platforms. The driver is only for re-configuring FPGAs and
there can be unlimited variations of different PCIe devices implemented
in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for
which an address range cannot be assigned on embedded 32-bit
platforms. pcim_enable_device() will fail here complaining about
not claimed BAR, even if the concerned BAR is not needed for FPGA
configuration at all. This makes the driver unusable.
>> + dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
>> + ret = -ENODEV;
>> + goto err;
>> + }
>> +
>> + conf->write_data = altera_cvp_write_data_iomem;
>> +
>
>> + conf->map = pci_iomap(pdev, CVP_BAR, 0);
>> + if (!conf->map) {
>> + dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
>
>Not needed in pcim_ case.
can I use other pcim_ functions if pcim_enalbe_device is not used?
>> + conf->write_data = altera_cvp_write_data_config;
>> + }
>
>> + conf->mgr = fpga_mgr_get(&pdev->dev);
>> + if (IS_ERR(conf->mgr)) {
>> + dev_err(&pdev->dev, "Getting fpga mgr reference failed\n");
>
>> + ret = -ENODEV;
>
>Why error is shadowed here?
oh, actually this mgr_get/mgr_put sequence is not needed here any more,
just forgot to remove it.
...
>> +module_init(altera_cvp_init);
>> +module_exit(altera_cvp_exit);
>
>I dunno for how many (3+ I think) years we have macros like
>module_pci_driver();
>
>Please, use it instead of above.
ok, will do.
Thanks,
Anatolij
Powered by blists - more mailing lists