Hey Chas,
There seems to be a fix me in this file in the function, solos_bh.
Is the default statement correct and I remove the fix me or
does it need to be rewritten.
Cheers Nick
On Sun, 20 Jul 2014 00:59:42 -0400
Nick Krause <[email protected]> wrote:
> Hey Chas,
> There seems to be a fix me in this file in the function, solos_bh.
> Is the default statement correct and I remove the fix me or
> does it need to be rewritten.
> Cheers Nick
>
I am afraid that I don't know enough about the solos hardware to know
if it needs to be rewritten. I gather the solos returns at least three
kinds of packets, data, status and command. If you wish to eliminate
the FIXME comment, you could just write:
@@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
dev_kfree_skb_any(skb);
break;
- case PKT_COMMAND:
- default: /* FIXME: Not really, surely? */
+ default: /* PKT_COMMAND */
if (process_command(card, port, skb))
break;
spin_lock(&card->cli_queue_lock);
and be done with it since that will preserve existing behavior.
On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR
<[email protected]> wrote:
> On Sun, 20 Jul 2014 00:59:42 -0400
> Nick Krause <[email protected]> wrote:
>
>> Hey Chas,
>> There seems to be a fix me in this file in the function, solos_bh.
>> Is the default statement correct and I remove the fix me or
>> does it need to be rewritten.
>> Cheers Nick
>>
>
> I am afraid that I don't know enough about the solos hardware to know
> if it needs to be rewritten. I gather the solos returns at least three
> kinds of packets, data, status and command. If you wish to eliminate
> the FIXME comment, you could just write:
>
> @@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
> dev_kfree_skb_any(skb);
> break;
>
> - case PKT_COMMAND:
> - default: /* FIXME: Not really, surely? */
> + default: /* PKT_COMMAND */
> if (process_command(card, port, skb))
> break;power
> spin_lock(&card->cli_queue_lock);
>
> and be done with it since that will preserve existing behavior.
My only question then is this function causing bugs as is?
Cheers Nick
On Mon, 21 Jul 2014 13:42:01 -0400
Nick Krause <[email protected]> wrote:
> On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR
> <[email protected]> wrote:
...
> > @@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
> > dev_kfree_skb_any(skb);
> > break;
> >
> > - case PKT_COMMAND:
> > - default: /* FIXME: Not really, surely? */
> > + default: /* PKT_COMMAND */
> > if (process_command(card, port, skb))
> > break;power
> > spin_lock(&card->cli_queue_lock);
> >
> > and be done with it since that will preserve existing behavior.
>
>
> My only question then is this function causing bugs as is?
> Cheers Nick
>
It has been there since the first version of this driver was released.
If it were breaking things, I would have to guess it would have been
noticed by now.
Just looking at the PKT_* defines, I would wildly guess that PKT_COMMAND
PKT_POPEN and PKT_PCLOSE are all being handled via the PKT_COMMAND|default
case. If not, popen and pclose would be leaking a skb for each usage
(which would be opening and closing the PVC -- not very often).
popen and pclose just seem like specialized PKT_COMMAND types.
process_command() woud ignore PKT_POPEN and PKT_PCLOSE since they aren't
long enough to contain a command. So maybe the comment should just go
away.
If you had access to hardware you could probably figure this out pretty
quickly. The programmer's manual doesn't seem to be available.
On Mon, Jul 21, 2014 at 2:18 PM, chas williams - CONTRACTOR
<[email protected]> wrote:
> On Mon, 21 Jul 2014 13:42:01 -0400
> Nick Krause <[email protected]> wrote:
>
>> On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR
>> <[email protected]> wrote:
> ...
>> > @@ -850,8 +850,7 @@ static void solos_bh(unsigned long card_arg)
>> > dev_kfree_skb_any(skb);
>> > break;
>> >
>> > - case PKT_COMMAND:
>> > - default: /* FIXME: Not really, surely? */
>> > + default: /* PKT_COMMAND */
>> > if (process_command(card, port, skb))
>> > break;power
>> > spin_lock(&card->cli_queue_lock);
>> >
>> > and be done with it since that will preserve existing behavior.
>>
>>
>> My only question then is this function causing bugs as is?
>> Cheers Nick
>>
>
> It has been there since the first version of this driver was released.
> If it were breaking things, I would have to guess it would have been
> noticed by now.
>
> Just looking at the PKT_* defines, I would wildly guess that PKT_COMMAND
> PKT_POPEN and PKT_PCLOSE are all being handled via the PKT_COMMAND|default
> case. If not, popen and pclose would be leaking a skb for each usage
> (which would be opening and closing the PVC -- not very often).
>
> popen and pclose just seem like specialized PKT_COMMAND types.
> process_command() woud ignore PKT_POPEN and PKT_PCLOSE since they aren't
> long enough to contain a command. So maybe the comment should just go
> away.
>
> If you had access to hardware you could probably figure this out pretty
> quickly. The programmer's manual doesn't seem to be available.
Seems based on your ideas , I can remove this FIX ME. I will send a patch
doing this.
Cheers Nick