On Wed, Jun 25, 2014 at 02:01:03PM -0500, Ken Cox wrote:
>
> 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 <j...@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.
Sounds great. Never add code to the kernel for "drivers that have not
been submitted", that's not ok as it's not how the kernel is developed.
greg k-h
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
