diff mbox

[v2,1/1] solos-pci: Fix regression introduced by newest firmware

Message ID 4D86AF4F.5010203@redfish-solutions.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Philip Prindeville March 21, 2011, 1:52 a.m. UTC
The newest FPGA firmware on the Solos processors correctly signals carrier transitions, bitrate, etc.

The driver previously ignored these messages, and the physical state was always ATM_PHY_SIG_UNKNOWN.

Now that the board reports its state, we expose a bug whereby the transition from UNKNOWN to LOST causes us to release all VC's.

We don't delete any VC's, but instead just send an indication of carrier change.

Signed-off-by: Philip A Prindeville <philipp@redfish-solutions.com>
---



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ben Hutchings March 21, 2011, 3:01 a.m. UTC | #1
On Sun, 2011-03-20 at 18:52 -0700, Philip Prindeville wrote:
> The newest FPGA firmware on the Solos processors correctly signals
> carrier transitions, bitrate, etc.
> 
> The driver previously ignored these messages, and the physical state
> was always ATM_PHY_SIG_UNKNOWN.
> 
> Now that the board reports its state, we expose a bug whereby the
> transition from UNKNOWN to LOST causes us to release all VC's.
> 
> We don't delete any VC's, but instead just send an indication of
> carrier change.
> 
> Signed-off-by: Philip A Prindeville <philipp@redfish-solutions.com>
> ---
> 
> --- a/drivers/atm/solos-pci.c	2011-03-20 15:27:40.000000000 -0600
> +++ b/drivers/atm/solos-pci.c	2011-03-20 16:32:11.000000000 -0600
> @@ -382,8 +382,10 @@ static int process_status(struct solos_c
> 
>   	/* Anything but 'Showtime' is down */
>   	if (strcmp(state_str, "Showtime")) {
>   		atm_dev_signal_change(card->atmdev[port], ATM_PHY_SIG_LOST);
> +#if 0
>   		atm_dev_release_vccs(card->atmdev[port]);
> +#endif

Either remove it or don't.  #if 0 is for people without version control.

Ben.

>   		dev_info(&card->dev->dev, "Port %d: %s\n", port, state_str);
> 		return 0;
> 	}
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller March 21, 2011, 4:57 a.m. UTC | #2
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 21 Mar 2011 03:01:36 +0000

> On Sun, 2011-03-20 at 18:52 -0700, Philip Prindeville wrote:
>> The newest FPGA firmware on the Solos processors correctly signals
>> carrier transitions, bitrate, etc.
>> 
>> The driver previously ignored these messages, and the physical state
>> was always ATM_PHY_SIG_UNKNOWN.
>> 
>> Now that the board reports its state, we expose a bug whereby the
>> transition from UNKNOWN to LOST causes us to release all VC's.
>> 
>> We don't delete any VC's, but instead just send an indication of
>> carrier change.
>> 
>> Signed-off-by: Philip A Prindeville <philipp@redfish-solutions.com>
>> ---
>> 
>> --- a/drivers/atm/solos-pci.c	2011-03-20 15:27:40.000000000 -0600
>> +++ b/drivers/atm/solos-pci.c	2011-03-20 16:32:11.000000000 -0600
>> @@ -382,8 +382,10 @@ static int process_status(struct solos_c
>> 
>>   	/* Anything but 'Showtime' is down */
>>   	if (strcmp(state_str, "Showtime")) {
>>   		atm_dev_signal_change(card->atmdev[port], ATM_PHY_SIG_LOST);
>> +#if 0
>>   		atm_dev_release_vccs(card->atmdev[port]);
>> +#endif
> 
> Either remove it or don't.  #if 0 is for people without version control.

Also, this would seem to break those using the older firmware.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philip Prindeville March 21, 2011, 5:56 a.m. UTC | #3
On 3/20/11 9:57 PM, David Miller wrote:
> From: Ben Hutchings<bhutchings@solarflare.com>
> Date: Mon, 21 Mar 2011 03:01:36 +0000
>
>> On Sun, 2011-03-20 at 18:52 -0700, Philip Prindeville wrote:
>>> The newest FPGA firmware on the Solos processors correctly signals
>>> carrier transitions, bitrate, etc.
>>>
>>> The driver previously ignored these messages, and the physical state
>>> was always ATM_PHY_SIG_UNKNOWN.
>>>
>>> Now that the board reports its state, we expose a bug whereby the
>>> transition from UNKNOWN to LOST causes us to release all VC's.
>>>
>>> We don't delete any VC's, but instead just send an indication of
>>> carrier change.
>>>
>>> Signed-off-by: Philip A Prindeville<philipp@redfish-solutions.com>
>>> ---
>>>
>>> --- a/drivers/atm/solos-pci.c	2011-03-20 15:27:40.000000000 -0600
>>> +++ b/drivers/atm/solos-pci.c	2011-03-20 16:32:11.000000000 -0600
>>> @@ -382,8 +382,10 @@ static int process_status(struct solos_c
>>>
>>>    	/* Anything but 'Showtime' is down */
>>>    	if (strcmp(state_str, "Showtime")) {
>>>    		atm_dev_signal_change(card->atmdev[port], ATM_PHY_SIG_LOST);
>>> +#if 0
>>>    		atm_dev_release_vccs(card->atmdev[port]);
>>> +#endif
>> Either remove it or don't.  #if 0 is for people without version control.
> Also, this would seem to break those using the older firmware.

It's not clear that dropping all VCs abruptly when carrier flapped was ever the right thing to do.

-Philip

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller March 21, 2011, 6:04 a.m. UTC | #4
From: Philip Prindeville <philipp_subx@redfish-solutions.com>
Date: Sun, 20 Mar 2011 22:56:43 -0700

> It's not clear that dropping all VCs abruptly when carrier flapped was
> ever the right thing to do.

So you've tested your change with the older firmware present?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philip Prindeville March 21, 2011, 7:25 a.m. UTC | #5
On 3/20/11 11:04 PM, David Miller wrote:
> From: Philip Prindeville<philipp_subx@redfish-solutions.com>
> Date: Sun, 20 Mar 2011 22:56:43 -0700
>
>> It's not clear that dropping all VCs abruptly when carrier flapped was
>> ever the right thing to do.
> So you've tested your change with the older firmware present?

I haven't, no: back-revving firmware has been known to brick cards.

I'm waiting to hear back from Guy and Nathan, they have old cards on hand that they can test.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse April 29, 2011, 11:09 p.m. UTC | #6
On Mon, 2011-03-21 at 00:25 -0700, Philip Prindeville wrote:
> On 3/20/11 11:04 PM, David Miller wrote:
> > From: Philip Prindeville<philipp_subx@redfish-solutions.com>
> > Date: Sun, 20 Mar 2011 22:56:43 -0700
> >
> >> It's not clear that dropping all VCs abruptly when carrier flapped was
> >> ever the right thing to do.
> > So you've tested your change with the older firmware present?
> 
> I haven't, no: back-revving firmware has been known to brick cards.
> 
> I'm waiting to hear back from Guy and Nathan, they have old cards on hand that they can test.

I have JTAG on one of mine and can test, but there's no real need in
this case. If you want to simulate the absence of the 'state changed'
notification, just *ignore* it when it does arrive.
diff mbox

Patch

--- a/drivers/atm/solos-pci.c	2011-03-20 15:27:40.000000000 -0600
+++ b/drivers/atm/solos-pci.c	2011-03-20 16:32:11.000000000 -0600
@@ -382,8 +382,10 @@  static int process_status(struct solos_c

  	/* Anything but 'Showtime' is down */
  	if (strcmp(state_str, "Showtime")) {
  		atm_dev_signal_change(card->atmdev[port], ATM_PHY_SIG_LOST);
+#if 0
  		atm_dev_release_vccs(card->atmdev[port]);
+#endif
  		dev_info(&card->dev->dev, "Port %d: %s\n", port, state_str);
		return 0;
	}