usb: dwc3: debugfs: Dump internal states

[Felipe Balbi <balbi@kernel.org>]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> +static int dwc3_internal_states_show(struct seq_file *s, void *unused)
>>> +{
>>> +	struct dwc3		*dwc = s->private;
>>> +	unsigned int		current_mode;
>>> +	unsigned long		flags;
>>> +	u32			reg;
>>> +
>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>> +	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>>> +	current_mode = DWC3_GSTS_CURMOD(reg);
>>> +
>>> +	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>> +
>>> +	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);
>> shouldn't the print be done with locks held?
>
> I think the lock for the prints should be held at a higher level.
> Otherwise, I don't think it will make a difference using dwc->lock.

what happens if this gets preempted when you release the lock and
a different thread writes to internal_states and reads the result before
$this thread?

>>> +static ssize_t dwc3_internal_states_write(struct file *file,
>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
>> why is this necessary? Seems like it would be nicer to create a
>> directory structure if the current operating mode is host so that we
>> don't need to write anything.
>
> Can you clarify more about the directory structure? Actually, I was
> wondering what's the best way to do this also. The reason I want to
> write to it is because the LSP selection for host is quite large (2^14).

right, turn each of those into a directory of its own. If you don't want
to or can't disclose proper names for those directories, just call them
lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
directory would write and read the correct registers.

Everything remains read-only.

> It doesn't make sense to dump everything if the controller is in host
> mode. So I force the user to write a specific LSP selection to dump at a
> time.

you can just as well force the use to read a file under a specific
directory. And if that same user really wants to read everything, it's
easy to do so with a simple loop to cat every file under every
directory.

Now, if directories would have a single file under, then you may decide
to, instead, create a single directory named e.g. lsp and put several
files under it. It's a matter of how much information you need to dump.

cheers