Message-ID: <d10df9d4-8cc7-b6f0-4096-cd0805407744@amd.com>
Date:   Tue, 16 May 2023 16:36:03 -0500
From:   Avadhut Naik <avadnaik@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     rafael@...nel.org, lenb@...nel.org, linux-acpi@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, yazen.ghannam@....com,
        linux-kernel@...r.kernel.org, Avadhut Naik <Avadhut.Naik@....com>
Subject: [RFC PATCH v1] ACPI: APEI: EINJ: Add support for vendor defined error
 types

Hi,

On 5/16/2023 14:25, Greg KH wrote:
> 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.
	Will do. Will have a separate patch for debugfs changes.
> 
> 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.
	I think we should be good in this case. The patch mainly attempts
to extend the functionality of einj module, if supported by the system.
The module itself, I think, requires for the debugfs to be mounted.

> 
> 
>>
>> Note: Some checkpatch warnings are ignored to maintain coding style.
> 
> That's not good, please follow the right style for new code.

	Noted. The only checkpatch warning that was ignored was pertaining
to the usage of S_IWUSR macro with debugfs_create_blob. Had noticed that a
majority of einj module's debugfs files have been created with S_IRUSR and
S_IWUSR macros. So used them to maintain uniformity.
	Will switch to octal permissions though.

Thanks,
Avadhut Naik

> 
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists