Re: [PATCH 4.19 099/243] usb: dwc3: debugfs: Properly print/set link state for HS

[Thinh Nguyen]

Hi,

Pavel Machek wrote:
> Hi!
>
>> From: Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx>
>>
>> [ Upstream commit 0d36dede457873404becd7c9cb9d0f2bcfd0dcd9 ]
>>
>> Highspeed device and below has different state names than superspeed and
>> higher. Add proper checks and printouts of link states for highspeed and
>> below.
> Just noticed some more oddity:
>> +	case DWC3_LINK_STATE_RESUME:
>> +		return "Resume";
>> +	default:
>> +		return "UNKNOWN link state\n";
>> +	}
> "UNKNOWN" would be consistent with the rest of the file.

Leaving the "link state" there may be fine for now due to the way it's 
printed in the log making it clearer.

>
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -433,13 +433,17 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>>   	unsigned long		flags;
>>   	enum dwc3_link_state	state;
>>   	u32			reg;
>> +	u8			speed;
>>   
>>   	spin_lock_irqsave(&dwc->lock, flags);
>>   	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>>   	state = DWC3_DSTS_USBLNKST(reg);
>> -	spin_unlock_irqrestore(&dwc->lock, flags);
>> +	speed = reg & DWC3_DSTS_CONNECTSPD;
>>   
>> -	seq_printf(s, "%s\n", dwc3_gadget_link_string(state));
>> +	seq_printf(s, "%s\n", (speed >= DWC3_DSTS_SUPERSPEED) ?
>> +		   dwc3_gadget_link_string(state) :
>> +		   dwc3_gadget_hs_link_string(state));
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>   
>>   	return 0;
>>   }
> The locking change is really wrong, right? There's no reason to do
> seq_printfs under spinlock..

Yes, it can be unlocked earlier.

>
>> @@ -477,6 +483,15 @@ static ssize_t dwc3_link_state_write(struct
> file *file,
>>   		return -EINVAL;
>>   
>>   	spin_lock_irqsave(&dwc->lock, flags);
>> +	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>> +	speed = reg & DWC3_DSTS_CONNECTSPD;
>> +
>> +	if (speed < DWC3_DSTS_SUPERSPEED &&
>> +	    state != DWC3_LINK_STATE_RECOV) {
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dwc3_gadget_set_link_state(dwc, state);
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>>   
> This might be ok but is not mentioned in the changelog.
>

I'll spell it out next time when I mention "add proper checks" in the 
commit message as it obviously wasn't clear.

Thanks,
Thinh