Prev: [PATCH -v3 2/2] irq_work, MCE: use irq_work in MCE
Next: Update the [register,unregister]_memory routines
From: Jeremy Fitzhardinge on 14 Jul 2010 14:10
On 07/14/2010 10:54 AM, David Miller wrote:
> And doing what you're doing is foolish on so many levels. One more
> duplication of code, one more place for unnecessary bugs to live, one
> more place that might need optimizations and thus require duplication
> of even more work people have done over the years.
>

Not to mention calling a function "MoveMemory" when it doesn't do a
memmove is just cruel.

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 14 Jul 2010 16:30
On Wed, Jul 14, 2010 at 10:18:22AM -0700, Pankaj Thakkar wrote:
> The plugin is guest agnostic and hence we did not want to rely on any
> kernel provided functions. The plugin uses only the interface provided
> by the shell.

Really? vmxnet3_plugin.c is no supposed to use any kernel-provided
functions at all? Then why have it in the kernel at all? Seriously,
why?

> The assumption is that since the plugin is really simple and straight
> forward (all the control/init complexity lies in the PF driver in the
> hypervisor) we should be able to get by for most of the things and for
> things like memcpy/memset the plugin can write simple functions like
> this.

If it's so simple, then why does it need to be separate? Why not just
put it in your driver as-is to handle the ring-buffer logic (as that's
all it looks to be doing), and then you don't need any plugin code at
all?

It looks like you are linking this file into your "main" driver module,
so I fail to see any type of separation at all happening with this
patch.

Or am I totally missing something here?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 14 Jul 2010 17:10
On Wed, Jul 14, 2010 at 01:42:59PM -0700, Shreyas Bhatewara wrote:
> +/* vmkernel and device backend shared definitions */
> +
> +#define VMXNET3_PLUGIN_NAME_LEN 256
> +#define VMXNET3_PLUGIN_REPOSITORY "/usr/lib/vmware/npa_plugins"

Why would the kernel care about this file path? And since when do we
hard-code file paths in the kernel in the first place (yeah, in some
places we do, but not like this...)

> +#define NPA_MEMIO_REGIONS_u64X 6
> +
> +typedef u32 VF_ID;
> +
> +struct Vmxnet3_VFInfo {
> + char pluginName[VMXNET3_PLUGIN_NAME_LEN];

This is never used.

> + u32 deviceInfo[VMXNET3_PLUGIN_INFO_LEN]; /* opaque data returned
> + * by PF driver */

This is happily copied around and zeroed out, but never actually used by
anything.


> + u64 memioAddr;
> + u32 memioLen;

This field is never used.

Why have fields in a structure that are never used?

> +};

<...>

> +/*
> + * Easy shell API calling macros.
> + */
> +#define Shell_AllocSmallBuffer(_state, _handle, _ringOffset) \
> + ((_state)->shellApi.allocSmallBuffer((_handle), (_ringOffset)))
> +#define Shell_AllocLargeBuffer(_state, _handle, _ringOffset) \
> + ((_state)->shellApi.allocLargeBuffer((_handle), (_ringOffset)))
> +#define Shell_FreeBuffer(_state, _handle, _ringOffset) \
> + ((_state)->shellApi.freeBuffer((_handle), (_ringOffset)))
> +#define Shell_CompleteSend(_state, _handle, _numPkt) \
> + ((_state)->shellApi.completeSend((_handle), (_numPkt)))
> +#define Shell_IndicateRecv(_state, _handle, _frame) \
> + ((_state)->shellApi.indicateRecv((_handle), (_frame)))
> +#define Shell_Log(_state, _loglevel, _n, _fmt, ...) \
> + do { \
> + if (logEnabled && (_loglevel) <= (u32)logLevel) { \
> + (_state)->shellApi.log((_n) + 1, \
> + "%s: " _fmt, \
> + __func__, \
> +##__VA_ARGS__); \
> + } \
> + } while (0)


This hiding of functions kind of implies that something odd is going on
here, right? At the least, make them inline functions so you get the
proper typechecking warnings/errors in a format that you can understand.

> +/*
> + * Some standard definitions
> + */
> +#ifndef NULL
> +#define NULL (void *)0
> +#endif

What's wrong with the kernel-provided version of this?

> +/*
> + * Utility macro to write a register's value (BAR0)
> + */
> +#define VMXNET3_WRITE_REG(_state, _offset, _value) \
> + (*(u32 *)((u8 *)(_state)->memioAddr + (_offset)) = \
> + (_value))

This will never work, sorry. Please use the proper functions for doing
this type of access. I'm amazed that anyone even thought this would
succeed...

> +/*
> + * Utility macro to align a virtual address
> + */
> +#define ALIGN_VA(_ptr, _align) ((void *)(((uintptr_t)(_ptr) + ((_align) - 1)) &\
> + ~((_align) - 1)))

What's wrong with the kernel provided function for this?

Anyway, just randomly poking at the code like this turns up these types
of trivial issues, has this code ever been run?

wierd,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: [PATCH -v3 2/2] irq_work, MCE: use irq_work in MCE
Next: Update the [register,unregister]_memory routines