* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
@ 2013-07-26 23:41 ` Dan Carpenter
2013-07-27 20:39 ` govind
2013-07-29 11:10 ` Dan Carpenter
` (6 subsequent siblings)
7 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2013-07-26 23:41 UTC (permalink / raw)
To: Kevin Curtis
Cc: David S. Miller, netdev@vger.kernel.org,
kernel-janitors@vger.kernel.org
On Fri, Jul 26, 2013 at 11:13:25AM +0100, Kevin Curtis wrote:
> Hi David,
> I'd like to update the FarSync driver for our T-Series cards
> in the latest Linux Kernel and I would also like to add a new
> driver for our FarSync Flex USB device.
>
> Should I direct the patch to you?
>
Read Documentation/SubmittingPatches
Use ./scripts/get_maintainer.pl to find who needs to be CC'd.
If you want, you can send your first attempt to kernel-janitors
without CC'ing Dave and netdev and we will tell you which bits of
SubmittingPatches you have missed.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
2013-07-26 23:41 ` Dan Carpenter
@ 2013-07-29 11:10 ` Dan Carpenter
2013-07-29 11:42 ` Dan Carpenter
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2013-07-29 11:10 UTC (permalink / raw)
To: kernel-janitors
So the main and worst news that I have is that this patch is going
to need to be broken up into about 10-20 little patches that each do
one thing and have description of what they are doing. The kernel
still has to compile and work correctly after each little patch is
applied (we call that bisectability because it helps "git bisect" to
work).
I know that breaking the patch up into tons of little patches is a
lot of work. Also you should know that I don't maintain any network
drivers so if you want you could try sending a huge patch and maybe
they will apply it, but probably there is no way. Anyway, sorry to
be the bearer of bad news here.
> The drivers have been derived from a version that we distribute
> with our cards and these drivers were originally written to
> support all known kernel versions.
Obviously, it's important for hardware vendors to care about
supporting older code, but for the mainline kernel we don't care
about that. We do have linux-backports-modules (have you looked at
this? It's the modern way of supporting new hardware with old
kernels.) Anyway, I haven't looked through the code enough to say
how much is compat layers for older kernels, it's just that I don't
want you to be surprised if the compat code is not accepted.
Next, the format of the patch needs to be something which "git am"
can apply automatically. The subject needs to be:
[patch 1/xx] farsync: blah blah blah.
There needs to be a patch description and a signed off by. The
patch needs to be inline instead of as an attachment. Verify that
it applies correctly before you send it to the list. Email it to
yourself, save the raw email as text, cat it to "git am".
There are a lot of scripts/checkpatch.pl warnings which will all
need to be fixed.
total: 692 errors, 907 warnings, 17158 lines checked
These are trivial formatting things so they are easy to fix.
I will look through the diff as well and see if I spot anything
obvious. I'm not a networking guy so I will miss a lot of things.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
2013-07-26 23:41 ` Dan Carpenter
2013-07-29 11:10 ` Dan Carpenter
@ 2013-07-29 11:42 ` Dan Carpenter
2013-07-29 12:58 ` Kevin Curtis
` (4 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2013-07-29 11:42 UTC (permalink / raw)
To: kernel-janitors
Oh, in my previous email I forgot to say that the patches should go
to netdev with Dave Miller CC'd.
@@ -20,18 +20,29 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/version.h>
-#include <linux/pci.h>
+#include <linux/fs.h>
#include <linux/sched.h>
-#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/pci.h>
There is no reason to shift the pci.h include around. Don't do
that.
+#define FST_DRIVER_TYPE "WAN"
+
+
Don't put two blank lines in a row.
+module_param(fst_iocinfo_version, int, S_IRUGO);
I don't understand what this is for. Probably it doesn't need to
be configurable.
Btw, here we are moving the module_param definitions around and
mostly they are unchanged. When you are breaking the patch into
lots of patches that can be one of the early ones:
[patch 3/xx] farsight: move some definitions around
Moving the definitions in a separate patch is easy to review and it
makes the next patch (where you add in a few more definitions), it
makes that patch much smaller and easier to review as well.
+typedef struct class class_t;
+#define CLASS_DEVICE_CREATE(class, dev, device, fmt, rest) device_create(class, device, dev, NULL, fmt, rest)
checkpatch will complain about the class_t typedef, that's not how
typedefs are used in the kernel. Also it's too generic a name.
Also don't do that #define just call device_create() directly.
/* Card shared memory layout
* ============ */
-#pragma pack(1)
+#pragma pack(2)
Don't use #pragmas in the kernel. Use the __packed macro to define
packed structs.
struct foo {
char a;
int b;
} __packed;
+typedef struct fst_fifo {
+ u16 fifo_length;
+ u16 read_idx;
+ u16 write_idx;
+ short overflow_idx;
+ u8 data[4];
+} FIFO, *PFIFO;
Typedef. "FIFO" and "PFIFO" are terrible names and way too generic.
Use "struct fst_fifo" throughout.
+/* Printing short cuts
+ */
+#define printk_err(fmt,A...) printk ( KERN_ERR FST_NAME ": " fmt, ## A )
+#define printk_warn(fmt,A...) printk ( KERN_WARNING FST_NAME ": " fmt, ## A )
+#define printk_info(fmt,A...) printk ( KERN_INFO FST_NAME ": " fmt, ## A )
+
This seems like a compat layer. Checkpatch will hopefully complain
about this.
/*
* PCI ID lookup table
*/
static DEFINE_PCI_DEVICE_TABLE(fst_pci_dev_id) = {
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T2P},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T4P},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T1U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T1U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T2U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T4U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_TE1, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_TE1},
+ {
+#ifdef FSC_TXP_SUPPORT
+ PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID,
+ PCI_ANY_ID, 0, 0, FST_TYPE_T2P}, {
+ PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID,
+ PCI_ANY_ID, 0, 0, FST_TYPE_T4P}, {
+#endif
The original formatting was way better here.
Anyway, I feel like I have given you a lot to work on.
The good news is that this patch is mostly about adding a new driver
and that can be done in one patch.
I saw later on this driver creates a /proc file. That should
probably sysfs file instead. We don't add /proc files these days.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
` (2 preceding siblings ...)
2013-07-29 11:42 ` Dan Carpenter
@ 2013-07-29 12:58 ` Kevin Curtis
2013-07-31 13:55 ` Kevin Curtis
` (3 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Kevin Curtis @ 2013-07-29 12:58 UTC (permalink / raw)
To: kernel-janitors
Hi Dan,
Thanks for the comments so far. I'll start by working out how to spilt the patch up and then work on the individual comments.
Regards
Kevin
-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: 29 July 2013 12:43
To: Kevin Curtis
Cc: kernel-janitors@vger.kernel.org; Dermot Smith
Subject: Re: Updated FarSync driver
Oh, in my previous email I forgot to say that the patches should go to netdev with Dave Miller CC'd.
@@ -20,18 +20,29 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/version.h>
-#include <linux/pci.h>
+#include <linux/fs.h>
#include <linux/sched.h>
-#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/pci.h>
There is no reason to shift the pci.h include around. Don't do that.
+#define FST_DRIVER_TYPE "WAN"
+
+
Don't put two blank lines in a row.
+module_param(fst_iocinfo_version, int, S_IRUGO);
I don't understand what this is for. Probably it doesn't need to be configurable.
Btw, here we are moving the module_param definitions around and mostly they are unchanged. When you are breaking the patch into lots of patches that can be one of the early ones:
[patch 3/xx] farsight: move some definitions around
Moving the definitions in a separate patch is easy to review and it makes the next patch (where you add in a few more definitions), it makes that patch much smaller and easier to review as well.
+typedef struct class class_t;
+#define CLASS_DEVICE_CREATE(class, dev, device, fmt, rest)
+device_create(class, device, dev, NULL, fmt, rest)
checkpatch will complain about the class_t typedef, that's not how typedefs are used in the kernel. Also it's too generic a name.
Also don't do that #define just call device_create() directly.
/* Card shared memory layout
* ============ */
-#pragma pack(1)
+#pragma pack(2)
Don't use #pragmas in the kernel. Use the __packed macro to define packed structs.
struct foo {
char a;
int b;
} __packed;
+typedef struct fst_fifo {
+ u16 fifo_length;
+ u16 read_idx;
+ u16 write_idx;
+ short overflow_idx;
+ u8 data[4];
+} FIFO, *PFIFO;
Typedef. "FIFO" and "PFIFO" are terrible names and way too generic.
Use "struct fst_fifo" throughout.
+/* Printing short cuts
+ */
+#define printk_err(fmt,A...) printk ( KERN_ERR FST_NAME ": " fmt, ## A )
+#define printk_warn(fmt,A...) printk ( KERN_WARNING FST_NAME ": " fmt, ## A )
+#define printk_info(fmt,A...) printk ( KERN_INFO FST_NAME ": " fmt, ## A )
+
This seems like a compat layer. Checkpatch will hopefully complain about this.
/*
* PCI ID lookup table
*/
static DEFINE_PCI_DEVICE_TABLE(fst_pci_dev_id) = {
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T2P},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T4P},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T1U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T1U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T2U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4U, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_T4U},
-
- {PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_TE1, PCI_ANY_ID,
- PCI_ANY_ID, 0, 0, FST_TYPE_TE1},
+ {
+#ifdef FSC_TXP_SUPPORT
+ PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID,
+ PCI_ANY_ID, 0, 0, FST_TYPE_T2P}, {
+ PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID,
+ PCI_ANY_ID, 0, 0, FST_TYPE_T4P}, {
+#endif
The original formatting was way better here.
Anyway, I feel like I have given you a lot to work on.
The good news is that this patch is mostly about adding a new driver and that can be done in one patch.
I saw later on this driver creates a /proc file. That should probably sysfs file instead. We don't add /proc files these days.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
` (3 preceding siblings ...)
2013-07-29 12:58 ` Kevin Curtis
@ 2013-07-31 13:55 ` Kevin Curtis
2013-07-31 14:50 ` Dan Carpenter
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Kevin Curtis @ 2013-07-31 13:55 UTC (permalink / raw)
To: kernel-janitors
Hi Dan,
I am fixing the errors and warning reported by checkpatch. I have quite a few of these:
ERROR: do not initialise globals to 0 or NULL
#90: FILE: drivers/net/wan/farsync.c:83:
+int fst_excluded_cards = 0;
Is this because when the module is loaded the globals are guaranteed to be zero'd already, or is the module init is expected to initialise them?
Regards
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
` (4 preceding siblings ...)
2013-07-31 13:55 ` Kevin Curtis
@ 2013-07-31 14:50 ` Dan Carpenter
2013-08-30 20:53 ` Dan Carpenter
2013-08-30 21:05 ` Dan Carpenter
7 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2013-07-31 14:50 UTC (permalink / raw)
To: kernel-janitors
On Wed, Jul 31, 2013 at 02:55:50PM +0100, Kevin Curtis wrote:
> Hi Dan,
> I am fixing the errors and warning reported by checkpatch. I have quite a few of these:
>
> ERROR: do not initialise globals to 0 or NULL
> #90: FILE: drivers/net/wan/farsync.c:83:
> +int fst_excluded_cards = 0;
>
> Is this because when the module is loaded the globals are
> guaranteed to be zero'd already, or is the module init is expected
> to initialise them?
>
In C globals are always initialized to zero at the start.
I think there was a reason to prefer one way over the other with
older versions of GCC but now it's just a style preference or
something.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
` (5 preceding siblings ...)
2013-07-31 14:50 ` Dan Carpenter
@ 2013-08-30 20:53 ` Dan Carpenter
2013-08-30 21:05 ` Dan Carpenter
7 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2013-08-30 20:53 UTC (permalink / raw)
To: kernel-janitors
On Fri, Aug 30, 2013 at 02:50:13PM +0100, Kevin Curtis wrote:
> Hi Dan,
> I have been working on getting the patches in to shape over the last month, and I think I have something that I hope will be acceptable. I have reduced the
>
> total: 692 errors, 907 warnings
>
> to
>
> total: 0 errors, 52 warnings.
>
> I thought I'd run it by the Janitors one more time before I formally submit it.
>
> There is now a set of 7 patches.
>
> 1 for the pci_ids.h file to add our new ids.
> And then 5 files that modify existing files and create new ones.
> And then 1 to update the Kconfig and Makefile.
>
Much better, but there are still issues.
Anyway, the first trivial thing is that these need to be sent inline as
a patch series. Everyone has macros to apply patches automatically from
email so these need to be in the normal format for "git am". Mail them
to yourself. Save the raw text including headers.
`cat email.txt | git am`. There are tuturials on how to mail git patch
series online.
Next, also very trivial, is that the patches are in the wrong order so the
compile breaks if I only apply the first two. That's not allowed.
I help review staging patches so when I see something like
farsync_patch_1 and farsync_patch_2 the patch is supposed to be adding a
feature. There are a lot of lines which start with '-' in the changelog
and I would question those. A lot of them are trivial white space
changes. If this were a staging patch I might ask that the white space
changes be pulled out into a separate patch. You can do this easily
with "git citool". Highlight the white space changes and right click
and select "stage lines for commit".
The other thing to be aware of is that we are coming up on a merge
window so it might already be too late to get this merged in the up
coming v3.12 kernel in November so you'll have to target v3.13 in Feb.
Maybe just submit it to the netdev ASAP and see what people say.
I think they will say that you need to reuse normal kernel CRC
functions and FIFO api. They might say that farsync_patch_1 needs to be
a separate patch for each paragraph in the changelog. They probably
have other comments as well.
But basically you've reached the end of kernel-janitor review. :) Good
luck.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: Updated FarSync driver
2013-07-26 10:13 ` Updated FarSync driver Kevin Curtis
` (6 preceding siblings ...)
2013-08-30 20:53 ` Dan Carpenter
@ 2013-08-30 21:05 ` Dan Carpenter
7 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2013-08-30 21:05 UTC (permalink / raw)
To: kernel-janitors
On Fri, Aug 30, 2013 at 11:53:02PM +0300, Dan Carpenter wrote:
> The other thing to be aware of is that we are coming up on a merge
> window so it might already be too late to get this merged in the up
> coming v3.12 kernel in November so you'll have to target v3.13 in Feb.
> Maybe just submit it to the netdev ASAP and see what people say.
Btw, submit the driver as soon as possible even though you are targeting
Feb. It goes into linux-next and people review it and run static
checkers on it. It's like there is a queue of patches. You can join
the queue any time but it takes 3-6 months before your patches come out
the other end and are released in an official kernel.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread