[Patch v3 2/4] Staging: unisys: Fix sparse warnings in uislib

[Ken Cox]

On 06/17/2014 06:01 PM, Greg KH wrote:
> On Thu, Jun 05, 2014 at 01:56:16PM -0500, Ken Cox wrote:
>> Added I/O version for the function ultra_vbus_init_channel() to get rid
>> of noderef sparse warnings when accessing I/O space.
>>
>> Signed-off-by: Ken Cox <jkc at redhat.com>
>> ---
>>   .../common-spar/include/channels/vbuschannel.h     | 40 ++++++++++++++++++++--
>>   drivers/staging/unisys/uislib/uislib.c             |  2 +-
>>   2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/unisys/common-spar/include/channels/vbuschannel.h b/drivers/staging/unisys/common-spar/include/channels/vbuschannel.h
>> index 000182c..af5a1ff 100644
>> --- a/drivers/staging/unisys/common-spar/include/channels/vbuschannel.h
>> +++ b/drivers/staging/unisys/common-spar/include/channels/vbuschannel.h
>> @@ -95,12 +95,46 @@ typedef struct _ULTRA_VBUS_CHANNEL_PROTOCOL {
>>   #define VBUS_CH_SIZE(MAXDEVICES) COVER(VBUS_CH_SIZE_EXACT(MAXDEVICES), 4096)
>>   
>>   static inline void
>> -ultra_vbus_init_channel(ULTRA_VBUS_CHANNEL_PROTOCOL __iomem *x,
>> -			int bytesAllocated)
>> +ultra_vbus_init_channel(ULTRA_VBUS_CHANNEL_PROTOCOL *x,
>> +			int bytes)
>>   {
>>   	/* Please note that the memory at <x> does NOT necessarily have space
>>   	* for DevInfo structs allocated at the end, which is why we do NOT use
>> -	* <bytesAllocated> to clear. */
>> +	* <bytes> to clear. */
>> +	memset(x, 0, sizeof(ULTRA_VBUS_CHANNEL_PROTOCOL));
>> +	if (bytes < (int) sizeof(ULTRA_VBUS_CHANNEL_PROTOCOL))
>> +		return;
>> +
>> +	x->ChannelHeader.VersionId = ULTRA_VBUS_CHANNEL_PROTOCOL_VERSIONID;
>> +	x->ChannelHeader.Signature = ULTRA_VBUS_CHANNEL_PROTOCOL_SIGNATURE;
>> +	x->ChannelHeader.SrvState = CHANNELSRV_READY;
>> +	x->ChannelHeader.HeaderSize = sizeof(x->ChannelHeader);
>> +	x->ChannelHeader.Size = bytes;
>> +	memcpy(&x->ChannelHeader.Type, &UltraVbusChannelProtocolGuid,
>> +		    sizeof(x->ChannelHeader.Type));
>> +	memcpy(&x->ChannelHeader.ZoneGuid, &NULL_UUID_LE, sizeof(uuid_le));
>> +	x->HdrInfo.structBytes = sizeof(ULTRA_VBUS_HEADERINFO);
>> +	x->HdrInfo.chpInfoByteOffset = sizeof(ULTRA_VBUS_HEADERINFO);
>> +	x->HdrInfo.busInfoByteOffset += sizeof(ULTRA_VBUS_DEVICEINFO);
>> +	x->HdrInfo.devInfoByteOffset += sizeof(ULTRA_VBUS_DEVICEINFO);
>> +	x->HdrInfo.deviceInfoStructBytes = sizeof(ULTRA_VBUS_DEVICEINFO);
>> +	bytes -= (sizeof(ULTRA_CHANNEL_PROTOCOL) +
>> +		  x->HdrInfo.devInfoByteOffset);
>> +	x->HdrInfo.devInfoCount = bytes / x->HdrInfo.deviceInfoStructBytes;
>> +}
> That's a _huge_ inline function, why is it inline?  Shouldn't it just be
> a "real" function instead?
>
> And given that you are duplicating most of the logic in
> ULTRA_VBUS_init_channel(), isn't there some other way to share the logic
> here?  Hm, no, I see why you did this, that's crazy.  The same logic to
> write to a structure or a iomemory-based structure?  Something feels
> really wrong here with what you are doing.  It's as if you init the
> channel, then write it all out to memory, right?
>
> No, wait, no one calls this function ever now.  Why is it even needed?
Originally, this function was here in preparation for an additional 
driver that has not been submitted.  After thinking about your comments 
and reviewing the code, I can make some changes to the calling functions 
and remove both versions of the function completely.