Warnings to ignore

Checkpatch produces many warnings; some of them are useful, and others are more noisy or taste-based.

You should avoid introducing any new checkpatch warnings about lines longer than 80 characters, but don't select existing warnings of that type to fix and submit patches for. In general, just picking a cutoff point and wrapping the line doesn't make for a good fix for these warnings; the result is often less readable than the original. The right fix is often to refactor or rewrite code to simplify it (for instance, factoring out functions, or changing if (success) { more code } to if (error) return ...;), and that's significantly more complex than other checkpatch cleanups.

Ignore checkpatch.pl if it complains about parens around boolean expressions or ternary conditionals in return values, like this:

        return ((depth > 1) ? (depth - 1) : depth);

sscanf

When changing code to check the return value of sscanf, make sure that you properly clean up before returning. Take, for example, this code in gdm_lte_tx() in drivers/staging/gdm724x/gdm_lte.c:

        if (nic_type & NIC_TYPE_ICMPV6) {
                if (gdm_lte_emulate_ndp(skb, nic_type) == 0) {
                        dev_kfree_skb(skb);
                        return 0;
                }
        }

        /*
        Need byte shift (that is, remove VLAN tag) if there is one
        For the case of ARP, this breaks the offset as vlan_ethhdr+4 is treated as ethhdr
        However, it shouldn't be a problem as the response starts from arp_hdr and ethhdr
        is created by this driver based on the NIC mac
        */
        if (nic_type & NIC_TYPE_F_VLAN) {
                struct vlan_ethhdr *vlan_eth = (struct vlan_ethhdr *)skb->data;
                nic->vlan_id = ntohs(vlan_eth->h_vlan_TCI) & VLAN_VID_MASK;
                data_buf = skb->data + (VLAN_ETH_HLEN - ETH_HLEN);
                data_len = skb->len - (VLAN_ETH_HLEN - ETH_HLEN);
        } else {
                nic->vlan_id = 0;
                data_buf = skb->data;
                data_len = skb->len;
        }

        /* If it is a ICMPV6 packet, clear all the other bits : for backward compatibility with the firmware */
        if (nic_type & NIC_TYPE_ICMPV6)
                nic_type = NIC_TYPE_ICMPV6;

        /* If it is not a dhcp packet, clear all the flag bits : original NIC, otherwise the special flag (IPVX | DHCP) */
        if (!(nic_type & NIC_TYPE_F_DHCP))
                nic_type &= NIC_TYPE_MASK;

        sscanf(dev->name, "lte%d", &idx);

When trying to fix the code to check the return value of sscanf, you may be tempted to simply write this patch:

@@ -444,7 +444,9 @@ static int gdm_lte_tx(struct sk_buff *skb, struct net_device *dev)
        if (!(nic_type & NIC_TYPE_F_DHCP))
                nic_type &= NIC_TYPE_MASK;

-       sscanf(dev->name, "lte%d", &idx);
+       ret = sscanf(dev->name, "lte%d", &idx);
+       if (ret != 1)
+               return -EINVAL;

        ret = nic->phy_dev->send_sdu_func(nic->phy_dev->priv_dev,
                                          data_buf, data_len,

However, this patch will cause a bug because it leaks memory. If you look carefully at the code before the sscanf call for return statements, you'll notice the code frees some memory before it returns:

        if (nic_type & NIC_TYPE_ICMPV6) {
                if (gdm_lte_emulate_ndp(skb, nic_type) == 0) {
                        dev_kfree_skb(skb);
                        return 0;
                }
        }

Therefore, your patch should also free this memory before it returns:

@@ -444,7 +444,9 @@ static int gdm_lte_tx(struct sk_buff *skb, struct net_device *dev)
        if (!(nic_type & NIC_TYPE_F_DHCP))
                nic_type &= NIC_TYPE_MASK;

-       sscanf(dev->name, "lte%d", &idx);
+       ret = sscanf(dev->name, "lte%d", &idx);
+       if (ret != 1) {
+               dev_kfree_skb(skb);
+               return -EINVAL;
+       }
        ret = nic->phy_dev->send_sdu_func(nic->phy_dev->priv_dev,
                                          data_buf, data_len,

You'll need to look very carefully at the code in between the last return statement and the sscanf to make sure further tear down is not needed. Sometimes looking at the tear-down in return statements after the sscanf may help.

Warning: Use of volatile is usually wrong

If you're not familiar with multithreaded programming, synchronization, and what "volatile" means in C, you should not work on this warning. Fixing this requires knowing whether the variables declared volatile are stored in or updated by a hardware device (in which case volatile might be legitimate), or if the driver is doing unusual things with multithreaded synchronization (usually better done some other way, such as atomic_t, but still requires significant care and analysis to fix).

Over 80-character line tips

The following guidelines can help you format the lines of new code you write to avoid triggering checkpatch's warning about lines longer than 80 characters. You should not, in general, select an existing 80-character warning to fix and submit a patch for. See the first section of this page for why.

Breaking comment lines

When breaking up comment lines that are over 80-characters, make sure to use the correct multi-line comment style, which is either:

        /* One line.
         * Two lines.
         */

or

        /*
         * One line
         * Two lines.
         */

Different driver authors use different styles, so use the style that is most common in the driver.

Breaking function calls

Sometimes a call to a function has several variables, and you need to break the line in the middle of those variables. Look at this example:

        pdata->urbdata = usb_alloc_coherent(pdata->udev, ACD_URB_BUFFER_LEN, GFP_KERNEL, &pdata->urb->transfer_dma);

This line is too long, so we want to break it up. By default, vim will increase the indentation of the trailing line by one tab:

        pdata->urbdata = usb_alloc_coherent(pdata->udev, ACD_URB_BUFFER_LEN,
                GFP_KERNEL, &pdata->urb->transfer_dma);

This style is fine, and generally perferred. However, some driver writers prefer to have the trailing line of a function call line up with the starting '('. They use tabs, followed by spaces, to align the trailing line:

        pdata->urbdata = usb_alloc_coherent(pdata->udev, ACD_URB_BUFFER_LEN,
                                            GFP_KERNEL,
                                            &pdata->urb->transfer_dma);

Again, the one tab indent style is preferred, but don't change lines that use the other style.

Breaking long math lines

When breaking up long math lines, make sure to break after an arthimetic or boolean operator, like this:

        really_long_function_call(other_long_function_call(variable) &&
                        yet_another_long_function_call(variable));

Breaking with dereferences or unary operators

Do not break lines at the dereference operator (->), even if it means leaving a very long line in.

Likewise, do not break a line between a unary operator (such as ! or *) and its argument.