1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
|
09:03 < wsa> welcome everyone to todays meeting
09:03 < wsa> here are the collected status updates
09:03 < wsa> Status updates
09:03 < wsa> ==============
09:03 < wsa> A - what have I done since last time
09:03 < wsa> ------------------------------------
09:03 < wsa> Geert
09:03 < wsa> : implemented explicit internal delay setting from DT for ravb, DT binding doc conversion to json-schema: ravb
09:03 < wsa> Shimoda-san
09:03 < wsa> : converted XHCI binding docs to YAML, fixed undefined temperature if negative for Thermal, and sent various series to fix mmc_suspend() with PSCI
09:03 < wsa> Ulrich
09:03 < wsa> : sent new series to handle WDT handover from bootloader which got merged, sent new series implementing atomic transfers for IIC
09:03 < wsa> Wolfram
09:03 < wsa> : removed final bits of i2c_new_device API and removed the old API so API conversion is now finished, reviewed slave implementation for SMBusHostNotify, and added the support for i2c-rcar, fixed various corner cases in the slave implemetation of i2c-rcar, wrote a new slave-backend to test rare I2C/SMBus features which usually need specific devices, tried to get an answer to the RPM
09:03 < wsa> refcounting issue, reviewed Uli's atomic transfer patch for IIC, minor treewide cleanup patches sent out while working on all of the above
09:03 < wsa> B - what I want to do until next time
09:03 < wsa> -------------------------------------
09:03 < wsa> Geert
09:03 < wsa> : wants to implement generic bindings for Ethernet MAC explicit internal delay setting
09:03 < wsa> Niklas
09:03 < wsa> : wants to eet up with Wolfram and flash Gen3 boards, and do the yearly test with SDIO-WIFI on Koelsch
09:03 < wsa> Shimoda-san
09:03 < wsa> : wants to keep working on mmc_suspend() with PSCI, discuss with Cogent about RAVB driver patch, convert usb-xhci, rcar-pci and sdhi DT docs to json-schema
09:03 < wsa> Ulrich
09:03 < wsa> : wants to sent new series implementing atomic transfers for IIC and I2C
09:03 < wsa> Wolfram
09:03 < wsa> : wants to meet with Niklas and fix Gen3 boards together, have two weeks of holidays, add SMBusAlert and I2C_M_RECV_LEN to both, the slave-testunit and the i2c-rcar driver, resend series 'fix stalled SCC' and 'implement manual calibration' for SDHI, review Shimoda-san's DMA unmapping series for SDHI
09:03 < wsa> C - problems I currently have
09:03 < wsa> -----------------------------
09:03 < wsa> Shimoda-san
09:03 < wsa> : wonders whether the mmc_suspend() improvement is really needed because we cannot avoid accidentally power off of eMMC around system suspend anyway
09:03 < wsa> Wolfram
09:03 < wsa> : asks how to ensure that atomic xfers on IIC have their clock enabled that late, and couldn't use Gen3 boards for a while so he had to switch to work which could be tested on Gen2
09:04 < wsa> shimoda: I assume with the RAVB patch you mentioned is about the "stop queues on timeout" issue?
09:05 < wsa> geertu: you mentioned that you'd apply Uli's RWDT clock patches but I don't see them in -next yet
09:05 < shimoda> wsa: yes
09:05 < geertu> wsa: They're in clk-renesas. Haven't sent a pull request to mturquette yet
09:06 < wsa> geertu: ah, they don't go directly into next from your branch?
09:06 * geertu doublechecks
09:06 < geertu> wsa: No they don't. Same with sh-pfc.
09:06 < wsa> shimoda: ok, thanks. will update that information
09:06 < shimoda> wsa: thanks!
09:07 < wsa> about the RPM get_sync ref-counting issue:
09:07 < geertu> shimoda: I assume s/Cogent/Sergei/?
09:07 < shimoda> geertu: yes
09:08 < wsa> I confirmed that always increasing the ref-count is intended behaviour. I have not confirmed yet, that it may be a better fix to simply remove the error checking instead of adding a put_noidle everywhere
09:08 < wsa> but from the last discussion, I thought for R-Car this is true?
09:09 < geertu> wsa: Yes, Rafael confirmed not checking is fine for us
09:09 < wsa> ok, nice to know
09:10 < wsa> so, i will ask people sending in such patches if they checked that removing the check may be the better solution
09:10 < geertu> https://lore.kernel.org/r/CAJZ5v0i87NGcy9+kxubScdPDyByr8ypQWcGgBFn+V-wDd69BHQ@mail.gmail.com
09:11 < geertu> "It is the right thing to do in the majority of cases."
09:11 < geertu> Now, there's still the bunch of "fixers" that will try to add error paths (and pm_runtime_put_*() calls) everywhere
09:12 < wsa> maybe we should add another function annotation: __must_not_check ;)
09:13 < wsa> at least, I now have a proper answer for them
09:13 < geertu> ;-)
09:14 < wsa> so, I have another topic but wanted to ask you guys first if there are questions regarding the status updates or in general?
09:14 < neg> wsa: is not __must_not_check equal to returning void :-)
09:15 < wsa> shimoda: I will seriously try to review the DMA unbalance patches before my holidays
09:15 < geertu> wsa: For atomic transfers, it's not just the clock, but also the power domain
09:15 < wsa> neg: you got it!
09:16 < wsa> geertu: yes, this was my remaining question
09:17 < shimoda> wsa: thanks!
09:17 < geertu> It seems some modules on R-Car Gen2 are "forgiving" when accessing them with the module clock disabled (i2c, rwdt)
09:17 < wsa> atomic_xfer happen very late, so clocks and power domains may be off already. and we can't use the calls to enable them anymore, right?
09:17 < geertu> Yes, pm_runtime_get_sync() may sleep
09:18 < geertu> We might set GENPD_FLAG_IRQ_SAFE for our domains
09:19 < wsa> but on what condition?
09:19 * kbingham discovers http://sweng.the-davies.net/Home/rustys-api-design-manifesto :-)
09:19 < geertu> kbingham: Yeah, pm_runtime_get_sync() is at -1 ;-)
09:19 < kbingham> geertu, That's the thread I found it from ;-)
09:20 < pinchartl> did Rafael provide any answer regarding whether pm_runtime_get_sync() should be fixed ?
09:20 < wsa> I thought it was -4
09:20 < geertu> there's also dev.power.irq_safe
09:21 < wsa> sometimes -7 :>
09:21 < geertu> pinchartl: Not to us. But according to the commit message for the initial clock fix, he doesn't intend to fix it.
09:22 < wsa> pinchartl: AFAIU it was expected to be the common case to NOT check the retval
09:22 < geertu> Correction: it's not in the commit message
09:22 < pinchartl> I don't see why we should fix it in drivers if he can't be bothered to fix it in runtime PM
09:22 < wsa> and it was intended that simple get/put-pairs without error checking simply should work
09:23 < geertu> https://lore.kernel.org/linux-pm/CAJZ5v0j+bsHaQcxK41yph8eRpMZ3DoerqA7uwS2B8De41Jwi7Q@mail.gmail.com/
09:23 < geertu> "I'd rather update the buggy callers than the ones that are OK.
09:23 < geertu> "
09:23 < geertu> But of course the "fixers" take (only) care of the latter
09:23 < pinchartl> wsa: amazing API design...
09:23 < wsa> pinchartl: ack
09:24 < pinchartl> ok, so I'll reject all patches that add error checks, easy
09:24 < wsa> my impression was that Rafael kind of regrets it today, but looks like "too late"
09:24 < geertu> it starts with a function that cannot fail.
09:24 < geertu> Then someone thinks it should not return void, but return an errorn code.
09:24 < geertu> Then all callers "must" be fixed
09:25 < geertu> wsa: indeed
09:25 < geertu> So the safe way is to always return an error code. At the expense of people checking for an error that cannot happen.
09:26 < geertu> The real bad thing here is that pm_runtime_get_sync() doesn't undo the refcounting on "failure"
09:26 < wsa> yep
09:27 < wsa> that is so counter-intuitive
09:28 < kbingham> I'm sure it could be fixed with some coccinelle and an intern ;-)
09:28 < wsa> I doubt coccinelle can find the few cases where an error code is useful
09:29 < wsa> geertu: about the atomic transfer issue
09:29 < wsa> does it mean PMIC drivers need to set the irq_safe flag?
09:30 < geertu> That flag would be for the PM domain
09:31 < geertu> oh, you mean dev.power.irq_safe
09:31 < wsa> whatever works, actually. but it should be the PMIC driver requesting the change, or?
09:31 < wsa> to save the DT description
09:32 < geertu> dev.power.irq_safe means "it is safe to run the ->runtime_suspend(), ->runtime_resume()
09:32 < geertu> and ->runtime_idle() callbacks for the given device in atomic context with
09:32 < geertu> interrupts disabled"
09:32 < geertu> but the doc for the flag says:
09:33 < geertu> "indicates that the callbacks will be invoked with the spinlock held and interrupts disabled", which is something different
09:33 < geertu> "is safe if" vs. "will"
09:34 < geertu> So if we mark all underlying infrastructure (PM Domain, i2c driver, pmic driver) irq_safe, we still have the might_sleep() call in pm_runtime_get_sync()
09:35 < geertu> My gut feeling says the only safe way to support this, is for the PMIC driver (which knows it's can be used to restart the system), to tell the i2c core that the i2c driver must stay awake for that.
09:35 < geertu> s/it's/it/
09:36 < wsa> can't we the PMIC driver just disable RPM for itself?
09:36 < wsa> and then this should propagate to all parents?
09:36 < geertu> That might work actually.
09:36 < geertu> Alternaively, is there something like an early restart notifier?
09:37 < geertu> Then the PMIC could wake up the i2c driver in that notifier, which would avoid the need for keeping the i2c driver awake all the time
09:38 < wsa> uli_: can you check these two options?
09:39 < wsa> other than that, last call for questions / comments
09:40 < uli_> i will check it out
09:40 < wsa> uli_: thanks!
09:40 < wsa> then I guess we can switch to the core meeting
|
