Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 21:25:24 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Avadhut Naik <Avadhut.Naik@amd.com>
Cc:     rafael@kernel.org, lenb@kernel.org, linux-acpi@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, yazen.ghannam@amd.com,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1] ACPI: APEI: EINJ: Add support for vendor defined
 error types
Message-ID: <2023051602-clear-encode-984e@gregkh>
References: <20230516183228.3736-1-Avadhut.Naik@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230516183228.3736-1-Avadhut.Naik@amd.com>
Precedence: bulk

On Tue, May 16, 2023 at 06:32:28PM +0000, Avadhut Naik wrote:
> According to ACPI specification 6.5, section 18.6.4, Vendor-Defined Error
> types are supported by the system apart from standard error types if bit
> 31 is set in the output of GET_ERROR_TYPE Error Injection Action. While
> the errors themselves and the length of their associated OEM Vendor data
> structure might vary between vendors, the physical address of this very
> structure can be computed through vendor_extension and length fields of
> SET_ERROR_TYPE_WITH_ADDRESS Data Structure and Vendor Error Type Extension
> Structure respectively (ACPI Spec 6.5, Table 18.31 and 18.32).
> 
> Currently, however, the einj module only computes the physical address of
> Vendor Error Type Extension Structure. Neither does it compute the physical
> address of OEM Vendor structure nor does it establish the memory mapping
> required for injecting Vendor-defined errors. Consequently, userspace
> tools have to establish the very mapping through /dev/mem, nopat kernel
> parameter and system calls like mmap/munmap initially before injecting
> Vendor-defined errors.
> 
> Circumvent the issue by computing the physical address of OEM Vendor data
> structure and establishing the required mapping with the structure. Create
> a new file "oem_error", if the system supports Vendor-defined errors, to
> export this mapping, through debugfs_create_blob API. Userspace tools can
> then populate their respective OEM Vendor structure instances and just
> write to the file as part of injecting Vendor-defined Errors.
> 
> Additionally, since the debugfs files created through debugfs_create_blob
> API are read-only, introduce a write callback to enable userspace tools to
> write OEM Vendor structures into the oem_error file.

When you say "additionally", that's usually a huge hint that you need to
split this up into multiple patches.

Please do so here.

Also note that debugfs is almost never a valid api for anything you care
about for having a running system, as it is locked down for root access
only and some distros refuse to enable it at all due to its security
leakage.  So be careful about creating an api here that you might need
to use on a normal running system.


> 
> Note: Some checkpatch warnings are ignored to maintain coding style.

That's not good, please follow the right style for new code.

thanks,

greg k-h