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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Dec 2019 19:35:59 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Pavel Machek <pavel@...x.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 099/243] usb: dwc3: debugfs: Properly print/set link
 state for HS

Hi,

Pavel Machek wrote:
> Hi!
>
>> From: Thinh Nguyen <thinh.nguyen@...opsys.com>
>>
>> [ 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