cvs commit: src/sys/dev/em if_em.c if_em.h

[Giorgos Keramidas]

On 2007-09-10 21:50, Jack F Vogel <jfv at freebsd.org> wrote:
> jfv         2007-09-10 21:50:40 UTC
>
>   FreeBSD src repository
>
>   Modified files:
>     sys/dev/em           if_em.c if_em.h
>   Log:
>   A number of small fixes:
>   - duplicate #define in header, thanks to Kevin Lo for pointing out.
>   - incorrect BUSMASTER enable logic, thanks Patrick Oeschger
>   - 82543 fails due to bogus IO BAR logic
>   - Allow 82571 to use MSI interrupts
>   - Checksum Offload for UDP not working on 82575

This is probably nit-picking but the following seems a bit odd:

% --- a/sys/dev/em/if_em.c Mon Sep 10 21:01:56 2007 +0000
% +++ b/sys/dev/em/if_em.c Mon Sep 10 21:50:40 2007 +0000
% @@ -2450,8 +2450,8 @@ em_identify_hardware(struct adapter *ada
%
%          /* Make sure our PCI config space has the necessary stuff set */
%          adapter->hw.bus.pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2);
% -        if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 &&
% -            (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN)) {
% +        if (!((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) &&
% +            (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN))) {
%                  device_printf(dev, "Memory Access and/or Bus Master bits "
%                      "were not set!\n");
%                  adapter->hw.bus.pci_cmd_word |=

It adds yet another pair of parentheses, just to use the style:

        if (!(condition1 && condition2))

which I sometimes find hard to read.  I'm not sure if this is commonly
the style used in our drivers, but isn't the following easier to parse?

%          /* Make sure our PCI config space has the necessary stuff set */
%          adapter->hw.bus.pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2);
% -        if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 &&
% -            (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN)) {
% +        if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 ||
% +            (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN) == 0) {
%                  device_printf(dev, "Memory Access and/or Bus Master bits "
%                      "were not set!\n");
%                  adapter->hw.bus.pci_cmd_word |=

AFAICT, the logic doesn't change, but not it is more explicitly clear
that any bit being unset triggers the rest of the code.  We also lose an
extra pair of parentheses, which makes the source code less "Lisp"y too :)

Having said that, I see that the '(if|while) \(!\(' pattern appears in
many other places:

keramida at kobe:/bsd/src$ egrep -r -e '(if|while) \([^!]\(' sys/dev | wc -l
     357
keramida at kobe:/bsd/src$

I also don't see any reference to this sort of construct in style(9), so
if this is the preferred style for FreeBSD code, then I need to learn to
read it without worrying too much :)

- Giorgos