linux-kernel - RE: [PATCH] IOSF: Add interface for the cases requiring fid

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]

Date:	Tue, 12 Apr 2016 00:18:36 +0000
From:	"Yang, Fei" <fei.yang@...el.com>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Fei Yang <fei.yang@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] IOSF: Add interface for the cases requiring fid

> In subject better to use x86/platform/iosf_mbi prefix.
Will update commit message.

>> Some implementations may require an additional step for setting the 
>> FID
> What FID stands for?
Function ID defined in IOSF specification. Will add more detail in the updated commit message.

>> Change-Id: Ic0227f9e74133a3203aa08e8471939f905d19622
> This should not be here.
Will remove

>> --- a/arch/x86/platform/intel/iosf_mbi.c
>> +++ b/arch/x86/platform/intel/iosf_mbi.c
>> @@ -98,6 +98,24 @@ fail_write:
>>  	return result;
>>  }
>>  
>> +static int iosf_mbi_pci_write_fid(u32 fid)
> Function name should continue already used pattern, i.e.
> …_write_mcrp()
Will change.

>> +{
>> +	int result;
>> +
>> +	if (!mbi_pdev)
>> +		return -ENODEV;
>> +


>> +	result = pci_write_config_dword(mbi_pdev, MBI_MCRP_OFFSET,
>> fid);

> The function of one line.
> So, please, inline it directly where it's used.
Not sure I understand this one, do you meant something like this?
static inline int iosf_mbi_pci_write_fid(u32 fid)


>> +/*
>> + * Some IP blocks require fid to access their registers.
> Any user?
> This API doesn't make much sense without user.
The driver that uses this API is a DRM based display controller driver, which is not currently in the upstream.
But this API is a prerequisite of pushing the display driver upstream.


>> + * fid value is programmed through MCRP register, see above function
>> + * iosf_mbi_pci_write_fid() for details.
>> + */
>> +int iosf_mbi_read_with_fid(u32 fid, u8 port, u8 opcode, u32 offset,
>> u32 *mdr)
> Name kinda fuzzy. How user should know which one to choose? Does fid ==
> 0 work for some cases? We have to think about API and naming.
This is SoC dependent. The drivers accessing registers through IOSF sideband has to
know if fid is required.


>> +	/*Access to the GFX unit is handled by GPU code */
> Spaces.
Will add.

>> +	/*Access to the GFX unit is handled by GPU code */
> Ditto.
Will add.

>> +	mcr = iosf_mbi_form_mcr(opcode, port, offset & MBI_MASK_LO);
>> +	mcrx = offset & MBI_MASK_HI;
>> +
>> +	spin_lock_irqsave(&iosf_mbi_lock, flags);
>> +	ret = iosf_mbi_pci_write_fid(fid);
>> +	if (!ret)
>> +		ret = iosf_mbi_pci_write_mdr(mcrx, mcr, mdr);
>> +	spin_unlock_irqrestore(&iosf_mbi_lock, flags);
> Both of them quite similar to already exist _write()/_read(). Is it possible to combine them out?
Trying to avoid modifying existing code that uses the _read/_write API.