netdev - Re: [PATCH v14 00/22] Type2 device basic support

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: <1bfb63d8-d16e-4779-b23a-8fbf34342ead@amd.com>
Date: Thu, 8 May 2025 15:55:10 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Alison Schofield <alison.schofield@...el.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v14 00/22] Type2 device basic support

>
> Hi Alejandro,


Hi Allison,


Thank you for your reviews.


My replies below.


>
> I commented on a few things in the individual patches and had a couple
> of series wide comments that I'll share here.
>
> I came into this set with the expectation that it was a model for Type 2
> device support and I expect it is, but without the summary I was hoping
> for. As I review the patches I can pick this stuff out, but would have
> appreciated each patch commit message stating its purpose more clearly.
> We get so use to reading this as a patchset, ie a short story, but once
> this set merges, the patches all must stand on their own, so including as
> much story in each commit message is needed. At the minimum, every patch to
> drivers/cxl/ in this set should explicitly state that this is being done in
> support of, or in preparation for, Type 2.


I understand and fully agree. I'll be sure about all patches clearly 
linked to the type2 support.


> I sorted the set into 3 buckets as I read.
>
> 1)These are the changes made to existing paths in the CXL driver to enable
>    Type 2. These change the existing driver behavior.
>
> 2)These are the patches that add new functionality to the CXL driver,
>    that is only exported for use by the Type 2 driver. These new
>    functions are not used in drivers/cxl/.
>
> 3)The sfx driver code. (which I glanced at)
>
> I'm wondering if bucket #2 is something to be documented in the
> Documentation/driver-api/cxl/.


It makes sense. But if I may say this, I think this could wait for a 
following-up as a Type2 driver guide including some of the problems I'm 
struggling with right now, not about the patchset itself, but some 
assumptions about what the BIOS does. Also, as the cover letter 
mentions, this is "basic support", so a first step which will have more 
steps following soon hopefully paving the proper road for this CXL 
functionality.


>
> Other general comments:
>
> - Consider each drivers/cxl/ function exported for use in sfc for a kernel
>    doc comment if not already present.


OK.


>
> - Some of the added Kernel-doc headers are missing  a Return: field


I'll check it.


>
> - "Printing numbers in parentheses (%d) adds no value and should be avoided."
>     See: Documentation/process/coding-style.rst.


I'll fix those.


>
> - Follow cxl conventions. Commit messages in drivers/cxl begine w uppercase


I'll fix those as well.


Thank you!


>
>
> -- Alison
>