Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach@linux.intel.com]

[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]



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@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.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@linux.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.
>