Re: [PATCH] tty: serial: msm_serial: Use DT aliases

[Frank Rowand]

On 11/13/2014 11:31 AM, Stephen Boyd wrote:
> On 11/12/2014 10:14 AM, Frank Rowand wrote:
>> On 11/10/2014 7:20 PM, Frank Rowand wrote:
>>> On 11/10/2014 6:07 PM, Stephen Boyd wrote:
>>>> On 11/10/2014 05:56 PM, Frank Rowand wrote:
>>>>> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>>>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>>>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>>>>>> --- a/drivers/tty/serial/msm_serial.c
>>>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>>>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>>>>>>  	const struct of_device_id *id;
>>>>>>  	int irq, line;
>>>>>>  
>>>>>> -	if (pdev->id == -1)
>>>>>> -		pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>>> -
>>>>>>  	if (pdev->dev.of_node)
>>>>>>  		line = of_alias_get_id(pdev->dev.of_node, "serial");
>>>>>>  	else
>>>>>>  		line = pdev->id;
>>>>>>  
>>>>>> +	if (line < 0)
>>>>>> +		line = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>>> +
>>>>>>  	if (unlikely(line < 0 || line >= UART_NR))
>>>>> Then this original check for "line < 0" can also be removed.
>>>>>
>>>>>
>>>> Well this matches what was there before. It would do atomic_inc_return
>>>> if the line was negative and then still check for a negative value. I
>>>> don't mind removing it though. Perhaps we should use an ida?:
>>>>
>>> OK, you are right.  If (pdev->id < -1) and (!pdev->dev.of_node) then
>>> the check is still needed.
>>>
>>> You could use an ida.  Some drivers use a bit map.  I really don't think
>>> this should become a complicated algorithm though.  If the rule is that
>>> either all UARTS have an alias, or no UART has an alias, then I think
>>> the patch could be something like the following.
>>>
>>> This combines your original patch, plus your fix patch, plus making
>>> the aliases all or nothing.  Not tested, not even compiled.
>>>
>>> What do you think?
>> < snip - previous version of patch >
>>
>> Previous version of the patch did not protect against no alias, followed
>> by alias.
>>
>>
>>
>> From: Frank Rowand <frank.rowand@xxxxxxxxxxxxxx>
>>
>> This patch is intended to show roughly what an implementation of an all or
>> nothing policy for using serialN aliases would look like.  It is intended
>> simply to help determine whether this policy should be used.
>>
>> Combines Stephen's first patch, Stephen's fix patch (to make the serialN
>> alias optional), plus makes use of the serialN alias all or nothing (either
>> all ports have an alias or none do).
>>
>> This is not a correct implementation because the atomic_read(&msm_uart_next_id)
>> does not protect against concurrency; instead a lock should be used.
>>
>> Not tested, not even compiled.
> 
> Sorry, I'm sort of lost. If there are serial aliases in the dts file,
> then we should alias all of the serial ports. If there aren't aliases
> then we're backwards compatible with the dts we have now and we'll do
> dynamic generation. Putting code into the driver to validate that this
> is true is not the job of the driver. If anything, it should validated
> when the dts file is created. If one day we screw up and have a dts file
> with such a bad configuration we'll have to work around it, but until
> that day comes I'd rather not think about it.

Maybe I did not understand when you said "Perhaps we should use an ida".
That sentence led me to think the driver should check for misconfiguration.
The case I was trying to handle was if there was at least one serialN
alias and at least one UART without an alias.  For example, if there
are three UARTs (serial_a, serial_b, serial_c, probed in that order)
and one alias (serial0 = &serial_c;) then the result would be:

   serial_a  line 0 (from msm_uart_next_id)
   serial_b  line 1 (from msm_uart_next_id)
   serial_c  line 0 (from the alias)

   Two UARTs probed with line == 0.  This is an error.

Most of the serial drivers don't check for this type of bad configuration.
Some drivers keep a bit map of which lines have been used.  I'm not sure
what they do in case of a conflict (I did not read to that level of detail).

I thought you were suggesting the driver check for the bad configuration,
so I was proposing a somewhat simple way of forcing a boot error for the
bad configuration.

Since you are not suggesting the driver check for the bad configuration,
you can ignore my proposal.  I agree that it is ok for the driver to
expect the board dts to be correct.  The problem should be detected by
the dts author on first boot as part of normal bring up testing, and
then corrected.

-Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/