Re: [v12, 7/8] base: soc: introduce soc_device_match() interface

[Peter Rosin]

On 2016-09-21 09:56, Alexander Shiyan wrote:
>> Среда, 21 сентября 2016, 9:57 +03:00 от Yangbo Lu :
>>
>> From: Arnd Bergmann < a...@arndb.de >
>>
>> We keep running into cases where device drivers want to know the exact
>> version of the a SoC they are currently running on. In the past, this has
>> usually been done through a vendor specific API that can be called by a
>> driver, or by directly accessing some kind of version register that is
>> not part of the device itself but that belongs to a global register area
>> of the chip.
> ...
>> +const struct soc_device_attribute *soc_device_match(
>> +const struct soc_device_attribute *matches)
>> +{
>> +int ret = 0;
>> +
>> +if (!matches)
>> +return NULL;
>> +
>> +while (!ret) {
>> +if (!(matches->machine || matches->family ||
>> +  matches->revision || matches->soc_id))
>> +break;
>> +ret = bus_for_each_dev(_bus_type, NULL, (void *)matches,
>> +   soc_device_match_one);
>> +if (!ret)
>> +matches++;
> 
> So, what happen if next "matches" (after increment) will be NULL?

A crash?

> I think you should use while(matches) at the start of this procedure.

*arrgh*

*If* matches wrap, you indeed have *big* problems. *Elsewhere*

Hint: Please read the review comments on the previous version of this
series [1] before commenting further.

Cheers,
Peter

[1] https://www.mail-archive.com/netdev@vger.kernel.org/msg126617.html


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count

[Peter Rosin]

On 2019-09-17 12:13, Uwe Kleine-König wrote:
> Hello Geert,
> 
> On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
>> Hi Rob, Uwe,
>>
>> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring  wrote:
>>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= 
>>>  wrote:
 Referencing device tree nodes from a property allows to pass arguments.
 This is for example used for referencing gpios. This looks as follows:

   gpio_ctrl: gpio-controller {
   #gpio-cells = <2>
   ...
   }

   someothernode {
   gpios = <_ctrl 5 0 _ctrl 3 0>;
   ...
   }

 To know the number of arguments this must be either fixed, or the
 referenced node is checked for a $cells_name (here: "#gpio-cells")
 property and with this information the start of the second reference can
 be determined.

 Currently regulators are referenced with no additional arguments. To
 allow some optional arguments without having to change all referenced
 nodes this change introduces a way to specify a default cell_count. So
 when a phandle is parsed we check for the $cells_name property and use
 it as before if present. If it is not present we fall back to
 cells_count if non-negative and only fail if cells_count is smaller than
 zero.

 Signed-off-by: Uwe Kleine-König 
>>
>> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
>> to non-negative cell_count") in robh/for-next, which causes a lock-up when
>> booting a shmobile_defconfig kernel on r8a7791/koelsch:
>>
>> rcu: INFO: rcu_sched self-detected stall on CPU
>> rcu: 0-: (2099 ticks this GP) idle=6fe/1/0x4002
>> softirq=29/29 fqs=1050
>>  (t=2100 jiffies g=-1131 q=0)
>> NMI backtrace for cpu 0
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
>> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
>> [] (show_stack) from [] (dump_stack+0x7c/0x9c)
>> [] (dump_stack) from [] (nmi_cpu_backtrace+0xa0/0xb8)
>> [] (nmi_cpu_backtrace) from [] 
>> (nmi_trigger_cpumask_backtrace+0x84/0x114)
>> [] (nmi_trigger_cpumask_backtrace) from [] 
>> (rcu_dump_cpu_stacks+0xac/0xc8)
>> [] (rcu_dump_cpu_stacks) from [] 
>> (rcu_sched_clock_irq+0x2ac/0x6b4)
>> [] (rcu_sched_clock_irq) from [] 
>> (update_process_times+0x30/0x5c)
>> [] (update_process_times) from [] 
>> (tick_nohz_handler+0xcc/0x120)
>> [] (tick_nohz_handler) from [] 
>> (arch_timer_handler_virt+0x28/0x30)
>> [] (arch_timer_handler_virt) from [] 
>> (handle_percpu_devid_irq+0xe8/0x21c)
>> [] (handle_percpu_devid_irq) from [] 
>> (generic_handle_irq+0x18/0x28)
>> [] (generic_handle_irq) from [] 
>> (__handle_domain_irq+0xa0/0xb4)
>> [] (__handle_domain_irq) from [] 
>> (gic_handle_irq+0x58/0x90)
>> [] (gic_handle_irq) from [] (__irq_svc+0x6c/0x90)
>> Exception stack(0xeb08dd30 to 0xeb08dd78)
>> dd20: c0cc7514 2013 0005 3b27
>> dd40: eb7c4020 c0cc750c 0051 0051 2013 c0c66b08 eb1cdc00 0018
>> dd60:  eb08dd80 c05c1a38 c0756c00 2013 
>> [] (__irq_svc) from [] 
>> (_raw_spin_unlock_irqrestore+0x1c/0x20)
>> [] (_raw_spin_unlock_irqrestore) from [] 
>> (of_find_node_by_phandle+0xcc/0xf0)
>> [] (of_find_node_by_phandle) from [] 
>> (of_phandle_iterator_next+0x68/0x178)
>> [] (of_phandle_iterator_next) from [] 
>> (of_count_phandle_with_args+0x5c/0x7c)
>> [] (of_count_phandle_with_args) from [] 
>> (i2c_demux_pinctrl_probe+0x24/0x1fc)
>> [] (i2c_demux_pinctrl_probe) from [] 
>> (platform_drv_probe+0x48/0x94)
>> [] (platform_drv_probe) from [] 
>> (really_probe+0x1f0/0x2b8)
>> [] (really_probe) from [] 
>> (driver_probe_device+0x140/0x158)
>> [] (driver_probe_device) from [] 
>> (device_driver_attach+0x44/0x5c)
>> [] (device_driver_attach) from [] 
>> (__driver_attach+0xac/0xb4)
>> [] (__driver_attach) from [] (bus_for_each_dev+0x64/0xa0)
>> [] (bus_for_each_dev) from [] 
>> (bus_add_driver+0x148/0x1a8)
>> [] (bus_add_driver) from [] (driver_register+0xac/0xf0)
>> [] (driver_register) from [] (do_one_initcall+0xa8/0x1d4)
>> [] (do_one_initcall) from [] 
>> (kernel_init_freeable+0x26c/0x2c8)
>> [] (kernel_init_freeable) from [] (kernel_init+0x8/0x10c)
>> [] (kernel_init) from [] (ret_from_fork+0x14/0x2c)
>> Exception stack(0xeb08dfb0 to 0xeb08dff8)
>> dfa0:    
>> dfc0:        
>> dfe0:     0013 
>>
>> Presumably it loops forever, due to a conversion of -1 to unsigned
>> somewhere?
> 
> Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
> of_count_phandle_with_args with cells_name=NULL. With that I don't see
> how my patch changes anything as the only change is