| 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: <alpine.DEB.2.22.394.2210071126060.2168979@rhweight-WRK1>
Date: Fri, 7 Oct 2022 11:35:47 -0700 (PDT)
From: matthew.gerlach@...ux.intel.com
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
cc: hao.wu@...el.com, yilun.xu@...el.com, russell.h.weight@...el.com,
basheer.ahmed.muddebihal@...el.com, trix@...hat.com,
mdf@...nel.org, linux-fpga@...r.kernel.org,
linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
tianfei.zhang@...el.com, corbet@....net,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-serial <linux-serial@...r.kernel.org>,
Jiri Slaby <jirislaby@...nel.org>, geert+renesas@...der.be,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
niklas.soderlund+renesas@...natech.se, macro@...am.me.uk,
johan@...nel.org, Lukas Wunner <lukas@...ner.de>
Subject: Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1
On Wed, 5 Oct 2022, Ilpo Järvinen wrote:
> Please try to remember cc all people who have commented your patches when
> sending the next version.
>
> On Tue, 4 Oct 2022, matthew.gerlach@...ux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
>>
>> Add generic support for MSIX interrupts for DFL devices.
>>
>> The location of a feature's registers is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address. Parse the location and pass the information
>> to DFL driver.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
>
>> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>> }
>>
>> static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> - resource_size_t ofst, u16 fid,
>> - unsigned int *irq_base, unsigned int *nr_irqs)
>> + resource_size_t ofst, struct dfl_feature_info *finfo)
>> {
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> enum dfl_id_type type;
>> - int virq;
>> - u64 v;
>> -
>> - type = feature_dev_id_type(binfo->feature_dev);
>> + u16 fid = finfo->fid;
>> + u64 v, dfh_ver;
>
> Drop dfh_ver.
I will drop dfh_ver.
>
>> + int virq, off;
>>
>> /*
>> * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL only provides mmio resources information for
>> + * current version, DFHv0, only provides mmio resources information for
>> * each feature in DFL Header, no field for interrupt resources.
>> * Interrupt resource information is provided by specific mmio
>> * registers of each private feature which supports interrupt. So in
>> * order to parse and assign irq resources, DFL framework has to look
>> * into specific capability registers of these private features.
>> *
>> - * Once future DFL version supports generic interrupt resource
>> - * information in common DFL headers, the generic interrupt parsing
>> - * code will be added. But in order to be compatible to old version
>> + * DFHv1 supports generic interrupt resource information in DFHv1
>> + * parameter blocks. But in order to be compatible to old version
>> * DFL, the driver may still fall back to these quirks.
>
> I'm not convinced this comment is useful as is after the introduction of
> v1. It feels too focused on v0 limitations.
>
> I suggest you move v0 limitations description to v0 block below and
> perhaps state in the end of it that comment that v1 is recommended for
> new things because it doesn't have those limitations. Or something along
> those lines.
I think I will rework the comment by splitting the descriptions for v0
and v1 and focusing on what each supports rather than limitations.
>
>> */
>> - if (type == PORT_ID) {
>> - switch (fid) {
>> - case PORT_FEATURE_ID_UINT:
>> - v = readq(base + PORT_UINT_CAP);
>> - ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> - inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +
>> + switch (finfo->dfh_version) {
>> + case 0:
>> + type = feature_dev_id_type(binfo->feature_dev);
>> + if (type == PORT_ID) {
>> + switch (fid) {
>> + case PORT_FEATURE_ID_UINT:
>> + v = readq(base + PORT_UINT_CAP);
>> + ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> + inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> + break;
>> + case PORT_FEATURE_ID_ERROR:
>> + v = readq(base + PORT_ERROR_CAP);
>> + ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> + inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> + break;
>> + }
>> + } else if (type == FME_ID) {
>> + if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> + v = readq(base + FME_ERROR_CAP);
>> + ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> + inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> + }
>> + }
>> + break;
>> +
>> + case 1:
>> + if (!dfhv1_has_params(base))
>> break;
>> - case PORT_FEATURE_ID_ERROR:
>> - v = readq(base + PORT_ERROR_CAP);
>> - ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> - inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +
>> + off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
>> + if (off < 0)
>> break;
>> - }
>> - } else if (type == FME_ID) {
>> - if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> - v = readq(base + FME_ERROR_CAP);
>> - ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> - inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> - }
>> +
>> + ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
>
> dfh_ver is uninitialized here. The compiler shouldn't have been happy with
> this.
I am surprised the compiler did not flag this uninitialized variable.
Getting rid of the dfh_ver altogether is the best course of action.
>
>> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>> if (binfo->len - ofst < size)
>> return -EINVAL;
>>
>> - ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> - if (ret)
>> - return ret;
>> -
>> finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>> if (!finfo)
>> return -ENOMEM;
>>
>> finfo->fid = fid;
>> finfo->revision = revision;
>> + finfo->dfh_version = dfh_version;
>> finfo->mmio_res.start = binfo->start + ofst;
>> finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> finfo->mmio_res.flags = IORESOURCE_MEM;
>> - finfo->irq_base = irq_base;
>> - finfo->nr_irqs = nr_irqs;
>> +
>> + ret = parse_feature_irqs(binfo, ofst, finfo);
>> + if (ret)
>> + return ret;
>
> finfo has to be freed in case of an error.
Good catch. Thanks.
>
> Thanks for rearranging, it looks more logical now.
>
> --
> i.
>
Powered by blists - more mailing lists