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
