Hi Andrew,
Thank you for reviewing patch 3/3. Please see my responses below.
> +int32_t zxdh_pf_pci_find_capability(struct pci_dev *pdev, uint8_t cfg_type,
> + uint32_t ioresource_types, int32_t *bars)
> +{
> + int32_t pos = 0;
> + uint8_t type = 0;
> + uint8_t bar = 0;
> +
> + for (pos = pci_find_capability(pdev, PCI_CAP_ID_VNDR); pos > 0;
> + pos = pci_find_next_capability(pdev, pos, PCI_CAP_ID_VNDR)) {
> + pci_read_config_byte(pdev, pos + offsetof
> (struct zxdh_pf_pci_cap, cfg_type), &type);
> + pci_read_config_byte(pdev, pos + offsetof
> (struct zxdh_pf_pci_cap, bar), &bar);
> Something odd going on with indentation? Has the mailer corrupted it?
The indentation in the original patch was correct (4-space). The mailer
appears to have corrupted the formatting in the display. Code is properly
indented in the actual patch.
> +
> + /* ignore structures with reserved BAR values */
> + if (bar > ZXDH_PF_MAX_BAR_VAL)
> + continue;
> +
> + if (type == cfg_type) {
> + if (pci_resource_len(pdev, bar) &&
> + pci_resource_flags(pdev, bar) & ioresource_types) {
> + *bars |= (1 << bar);
> + return pos;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +void __iomem *zxdh_pf_map_capability(struct dh_core_dev *dh_dev, int32_t off,
> + size_t minlen, uint32_t align,
> + uint32_t start, uint32_t size,
> + size_t *len, resource_size_t *pa,
> + uint32_t *bar_off)
> + p = pci_iomap_range(pdev, bar, offset, length);
> + if (unlikely(!p)) {
> Is this hot path? Please only use unlikely() when dealing with frames
> in the hot path.
Removed unlikely(). It's not in a hot path.
> +int32_t zxdh_pf_common_cfg_init(struct dh_core_dev *dh_dev)
> +{
> + int32_t common = 0;
> + struct zxdh_pf_device *pf_dev = dh_core_priv(dh_dev);
> + struct pci_dev *pdev = dh_dev->pdev;
> +
> + /* check for a common config: if not, use legacy mode (bar 0). */
> + common = zxdh_pf_pci_find_capability(pdev, ZXDH_PCI_CAP_COMMON_CFG,
> + IORESOURCE_IO | IORESOURCE_MEM,
> + &pf_dev->modern_bars);
> + if (common == 0) {
> + LOG_ERR("missing capabilities %i, leaving for legacy driver\
> n", common);
> + return -ENODEV;
> + }
> +
> + pf_dev->common = zxdh_pf_map_capability(dh_dev, common,
> + sizeof(struct zxdh_pf_pci_common_cfg),
> + ZXDH_PF_ALIGN4, 0,
> + sizeof(struct zxdh_pf_pci_common_cfg),
> + NULL, NULL, NULL);
> + if (unlikely(!pf_dev->common)) {
> + LOG_ERR("pf_dev->common is null\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +int32_t zxdh_pf_notify_cfg_init(struct dh_core_dev *dh_dev)
> +{
> + /* We don't know how many VQs we'll map, ahead of the time.
> + * If notify length is small, map it all now. Otherwise, map each VQ individually later.
> + */
> + if ((uint64_t)notify_length + (notify_offset % PAGE_SIZE) <= PAGE_SIZE) {
> Please try to avoid casts. They suggest the types are wrong. You will
> probably have better code if you don't need the cast.
Fixed. Changed notify_length and notify_offset to size_t type.
The cast is no longer needed:
if (notify_length + (notify_offset % PAGE_SIZE) <= PAGE_SIZE)
> +int32_t zxdh_pf_modern_cfg_init(struct dh_core_dev *dh_dev)
> +{
> + int32_t ret = 0;
> + struct zxdh_pf_device *pf_dev = dh_core_priv(dh_dev);
> + struct pci_dev *pdev = dh_dev->pdev;
> +
> + ret = zxdh_pf_common_cfg_init(dh_dev);
> + if (ret != 0) {
> if (ret)
> would be more normal.
Fixed. Changed to "if (ret)".
> +void zxdh_pf_get_vf_mac
> (struct dh_core_dev *dh_dev, uint8_t *mac, int32_t vf_id)
> +{
> + uint32_t DEV_MAC_L = 0;
> + uint16_t DEV_MAC_H = 0;
> + struct zxdh_pf_device *pf_dev = dh_core_priv(dh_dev);
> +
> + if (pf_dev->pf_sriov_cap_base) {
> + DEV_MAC_L = ioread32((void __iomem *)(pf_dev->pf_sriov_cap_base +
> + (pf_dev->sriov_bar_size) * vf_id +
> + pf_dev->dev_cfg_bar_off));
> Is the cast needed? pf_dev->pf_sriov_cap_base should already be void *
> __iomem.
Fixed. Removed the cast. pf_sriov_cap_base is already void __iomem *.
Best regards,
Junyang Han