netdev - Re: [PATCH v2 07/15] cxl: support type2 memdev creation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240804183139.000019e2@Huawei.com>
Date: Sun, 4 Aug 2024 18:31:39 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <alejandro.lucero-palau@....com>
CC: <linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<dan.j.williams@...el.com>, <martin.habets@...inx.com>,
	<edward.cree@....com>, <davem@...emloft.net>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <edumazet@...gle.com>, <richard.hughes@....com>,
	Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v2 07/15] cxl: support type2 memdev creation

On Mon, 15 Jul 2024 18:28:27 +0100
alejandro.lucero-palau@....com wrote:

> From: Alejandro Lucero <alucerop@....com>
> 
> Add memdev creation from sfc driver.
> 
> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
> creating a memdev leading to problems when obtaining cxl_memdev_state
> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
> managed by a specific vendor driver and does not need same sysfs files
> since not userspace intervention is expected. This patch checks for the
> right device type in those functions using cxl_memdev_state.
> 
> Signed-off-by: Alejandro Lucero <alucerop@....com>
Same general comment about treating failure to get things you expect
as proper driver probe errors.  Very unlikely we'd ever want to carry
on if these fail. If we do want to, that should be a high level decision
and the chances are the driver needs to know that the error occurred
so it can take some mitigating measures (using some alternative mechanisms
etc).

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index a84fe7992c53..0abe66490ef5 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -57,10 +57,16 @@ void efx_cxl_init(struct efx_nic *efx)
>  	if (cxl_accel_request_resource(cxl->cxlds, true))
>  		pci_info(pci_dev, "CXL accel resource request failed");
>  
> -	if (!cxl_await_media_ready(cxl->cxlds))
> +	if (!cxl_await_media_ready(cxl->cxlds)) {
>  		cxl_accel_set_media_ready(cxl->cxlds);
> -	else
> +	} else {
>  		pci_info(pci_dev, "CXL accel media not active");
> +		return;
Once you are returning an error in this path you can just have
		return -ETIMEDOUT; or similar here adn avoid
this code changing in this patch.
> +	}
> +
> +	cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds);
> +	if (IS_ERR(cxl->cxlmd))
> +		pci_info(pci_dev, "CXL accel memdev creation failed");

I'd treat this one as fatal as well.

People argue in favor of muddling on to allow firmware upgrade etc.
That is fine, but pass up the errors then decide to ignore them
at the higher levels.

>  }
>  
>