| 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: <CAHp75VfxGOw-G1+Cqk75eVK6EoLeRDemRc2zif1aJc2o+1fzMQ@mail.gmail.com>
Date: Fri, 2 Jun 2017 20:43:21 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Anatolij Gustschin <agust@...x.de>
Cc: linux-fpga@...r.kernel.org, Alan Tull <atull@...nel.org>,
Moritz Fischer <moritz.fischer@...us.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
matthew.gerlach@...ux.intel.com, yi1.li@...ux.intel.com
Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver
On Sun, May 14, 2017 at 6:51 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.
Few comments from me.
> +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);
Is it too far beyond 80 characters? I would leave it in one line (~83
characters are okay).
> + char mgr_name[64];
> + u8 numclks;
> +};
> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> + unsigned int i;
> + u32 val32;
Drop the useless suffix.
> +}
> +static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk,
> + u32 status_val, int timeout_us)
> +{
> + u32 val32;
Ditto.
> + if (!timeout_us)
> + return -ETIMEDOUT;
Hmm...
What as a user I would expect here is at least one attempt (0 -- no
timeout, but try once).
> +
> + do {
> + /* use small usleep value to re-check and break early */
> + usleep_range(10, 11);
> +
> + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> + if ((val32 & status_msk) == status_val)
> + return 0;
> +
> + timeout_us -= 10;
> + } while (timeout_us > 0);
> +
> + return -ETIMEDOUT;
> +}
> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + u32 val32;
Drop the suffix.
> + return ret;
> +}
> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + u32 val32;
Ditto.
> + int ret;
> +
> + /* clock to data ratio for uncompressed and unencrypted images */
> + conf->numclks = 1;
To else branch of below?
> + if (info) {
> + }
> + ret = altera_cvp_teardown(mgr, info);
> + if (ret < 0)
> + return ret;
What is the meaning of ret > 0?
Can't it be just if (ret) here?
> + ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
> + VSEC_CVP_STATUS_CFG_RDY, 10);
> + if (ret < 0) {
> + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
> + return ret;
> + }
Ditto.
Also check another code like this above and below.
> + 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;
Suffix.
> +}
> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + u32 status_msk;
status_mask
> + u32 val32;
Drop the suffix.
> +}
> +static ssize_t show_chkcfg(struct device_driver *dev, char *buf)
> +{
> + return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);
Just altera_cvp_chkcfg.
> +}
> +static struct pci_device_id altera_cvp_id_tbl[] = {
> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
Does it have dedicated PCI class?
PCI_ANY_ID usually is too broad.
> +static int altera_cvp_probe(struct pci_dev *pdev,
> + const struct pci_device_id *dev_id)
> +{
> + struct altera_cvp_conf *conf;
> + u16 cmd, val16;
Drop the suffix.
> + /*
> + * First check if this is the expected FPGA device. PCI config
> + * space access works without enabling the pci device, memory
pci -> PCI
> + * space access is enabled further down.
> + */
> + /*
> + * Enable memory BAR access. We cannot use pci_enable_device() here
> + * because it will make the driver unusable with FPGA devices that
> + * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit
iomem -> IOMEM
> + * platform. Such BARs will not have an assigned address range and
> + * pci_enable_device() will fail, complaining about not claimed BAR,
> + * even if the concerned BAR is not needed for FPGA configuration
> + * at all. Thus, enable the device via pci config space command.
pci -> PCI
> + */
> + ret = pci_request_region(pdev, CVP_BAR, "CVP");
> + if (ret < 0) {
if (ret)
> + dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> + goto err;
> + }
> + snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
> + ALTERA_CVP_MGR_NAME,
pdev->bus->number,
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
pci_name() ?
> +err_unmap:
> + pci_iounmap(pdev, conf->map);
> + pci_release_region(pdev, CVP_BAR);
> +err:
err_disable:
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists