Re: solos-pci.c: Fix me
On Mon, Jul 21, 2014 at 2:18 PM, chas williams - CONTRACTOR wrote: > On Mon, 21 Jul 2014 13:42:01 -0400 > Nick Krause wrote: > >> On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: solos-pci.c: Fix me
On Mon, 21 Jul 2014 13:42:01 -0400 Nick Krause wrote: > On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: solos-pci.c: Fix me
On Mon, Jul 21, 2014 at 9:14 AM, chas williams - CONTRACTOR wrote: > On Sun, 20 Jul 2014 00:59:42 -0400 > Nick Krause 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: solos-pci.c: Fix me
On Sun, 20 Jul 2014 00:59:42 -0400 Nick Krause 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
solos-pci.c: Fix me
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/