Date: Sat, 06 Sep 2008 19:19:33 -0700 From: Sean Bruno <sbruno@miralink.com> To: Dieter <freebsd@sopwith.solgatos.com> Cc: freebsd-firewire@freebsd.org Subject: Re: New and improved? patch Message-ID: <48C33A35.6090203@miralink.com> In-Reply-To: <200808311934.TAA10392@sopwith.solgatos.com> References: <200808311934.TAA10392@sopwith.solgatos.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> usage:
>
>> [-f force_root ]
>>
>
> Suggestion: [-f force_root_node ] or maybe "desired_root_node" or maybe
> "root_node" or maybe just "node", same as -c -d -o -s
>
>
I think the variable name is fine, but the man page should be expanded.
>> -g: broadcast gap_count by phy_config packet
>>
>
> Suggestion: -g: set gap_count by broadcasting phy_config packet
> or maybe just: -g: set gap_count
>
I agree. The description is deficient and should not only be expanded, but
should reference the IEEE specifications, i.e. 1394a-2005
>> -f: broadcast force_root by phy_config packet
>>
>
> Suggestion: -f force a node to become the root node by broadcasting phy_config packet
> or maybe just: -f force a node to become the root node
>
> 7.0 AMD64 # ./fwcontrol -f -5
> fwcontrol: main:set_root_node out of range: No such file or directory
>
> "No such file or directory" seems wrong
>
> 7.0 AMD64 # ./fwcontrol -g 70
> fwcontrol: main:set_gap_count out of range: No such file or directory
>
> "No such file or directory" seems wrong
>
> 7.0 AMD64 # ./fwcontrol -d 70
> fwcontrol: no such node -1.
>
> (a) 70 becomes -1 ?
> (b) Wouldn't -d have the same range as -f ?
>
> 7.0 AMD64 # ./fwcontrol -c 70
> fwcontrol: no such node -1.
>
> (a) 70 becomes -1 ?
> (b) Wouldn't -c have the same range as -f ?
>
> 7.0 AMD64 # ./fwcontrol -o 70
> fwcontrol: main: node out of range: 70
> : No such file or directory
>
> "No such file or directory" seems wrong
>
> 7.0 AMD64 # ./fwcontrol -s 70
> fwcontrol: main: node out of range: 70
> : No such file or directory
>
> "No such file or directory" seems wrong
>
>
>
> - asyreq->pkt.mode.ld[1] |= (root_node & 0x3f) << 24 | 1 << 23;
> + asyreq->pkt.mode.ld[1] |= ((root_node << 24) | (1 << 23));
> if (gap_count >= 0)
> - asyreq->pkt.mode.ld[1] |= 1 << 22 | (gap_count & 0x3f) << 16;
> + asyreq->pkt.mode.ld[1] |= ((1 << 22) | (gap_count << 16));
>
> Any reason for pulling out the "& 0x3f" ? Yeah it should be redundant
> now that there is range checking on the arguments, but as you said,
> fwcontrol is dangerous and the mask makes it safer at very little cost.
>
>
>
I find a range check to be an easier reference than a bit mask/shift
operation.
I'd like the next guy who comes through the code to be able to understand
the changes and the code path. Also, more comments are probably required.
> - for (i = 0; i < 4; i++) {
> - snprintf(name, sizeof(name), "%s.%d", devbase, i);
> - if ((*fd = open(name, O_RDWR)) >= 0)
> - break;
> - }
> + *fd = open(devname, O_RDWR);
>
>
> Looking at various firewire man pages, I don't find any explanation of
> the various /dev filenames, such as what the .%d part was/is for. So I
> have no clue why this code was changed. Did I miss a discussion?
>
I'm going to have to put a big "I have no idea" here. This predates my
attempts
at stabilization. Let's examine it further in the driver code. Perhaps
that will explain it's use.
--
Sean Bruno
MiraLink Corporation
6015 NE 80th Ave, Ste 100
Portland, OR 97218
Cell 503-358-6832
Phone 503-621-5143
Fax 503-621-5199
MSN: sbruno@miralink.com
Google: seanwbruno@gmail.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48C33A35.6090203>