| Message ID | 20200511123744.96246-1-christian.gmeiner@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/etnaviv: fix perfmon domain interation | expand |
Hi Christian, Am Montag, den 11.05.2020, 14:37 +0200 schrieb Christian Gmeiner: > The GC860 has one GPU device which has a 2d and 3d core. In this case > we want to expose perfmon information for both cores. > > The driver has one array which contains all possible perfmon domains > with some meta data - doms_meta. Here we can see that for the GC860 > two elements of that array are relevant: > > doms_3d: is at index 0 in the doms_meta array with 8 perfmon domains > doms_2d: is at index 1 in the doms_meta array with 1 perfmon domain > > The userspace driver wants to get a list of all perfmon domains and > their perfmon signals. This is done by iterating over all domains and > their signals. If the userspace driver wants to access the domain with > id 8 the kernel driver fails and returns invalid data from doms_3d with > and invalid offset. > > This results in: > Unable to handle kernel paging request at virtual address 00000000 > > On such a device it is not possible to use the userspace driver at all. > > The fix for this off-by-one error is quite simple. > > Reported-by: Paul Cercueil <paul@crapouillou.net> > Tested-by: Paul Cercueil <paul@crapouillou.net> > Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query infrastructure") > Cc: stable@vger.kernel.or Missing last letter of the TLD. > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > index e6795bafcbb9..35f7171e779a 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain *pm_domain(const struct etnaviv_gpu *gpu, > if (!(gpu->identity.features & meta->feature)) > continue; > > - if (meta->nr_domains < (index - offset)) { > + if ((meta->nr_domains - 1) < (index - offset)) { While the logic is correct, I find this quite hard to read. A more idiomatic way to write this (which is much easier to grok when reading the code IMHO) would be: if (index - offset >= meta->nr_domains) If you agree, please send a v2 of this patch. Regards, Lucas > offset += meta->nr_domains; > continue; > }
Hi Lucas, Am So., 17. Mai 2020 um 14:03 Uhr schrieb Lucas Stach <l.stach@pengutronix.de>: > > Hi Christian, > > Am Montag, den 11.05.2020, 14:37 +0200 schrieb Christian Gmeiner: > > The GC860 has one GPU device which has a 2d and 3d core. In this case > > we want to expose perfmon information for both cores. > > > > The driver has one array which contains all possible perfmon domains > > with some meta data - doms_meta. Here we can see that for the GC860 > > two elements of that array are relevant: > > > > doms_3d: is at index 0 in the doms_meta array with 8 perfmon domains > > doms_2d: is at index 1 in the doms_meta array with 1 perfmon domain > > > > The userspace driver wants to get a list of all perfmon domains and > > their perfmon signals. This is done by iterating over all domains and > > their signals. If the userspace driver wants to access the domain with > > id 8 the kernel driver fails and returns invalid data from doms_3d with > > and invalid offset. > > > > This results in: > > Unable to handle kernel paging request at virtual address 00000000 > > > > On such a device it is not possible to use the userspace driver at all. > > > > The fix for this off-by-one error is quite simple. > > > > Reported-by: Paul Cercueil <paul@crapouillou.net> > > Tested-by: Paul Cercueil <paul@crapouillou.net> > > Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query infrastructure") > > Cc: stable@vger.kernel.or > > Missing last letter of the TLD. > > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > > index e6795bafcbb9..35f7171e779a 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > > @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain *pm_domain(const struct etnaviv_gpu *gpu, > > if (!(gpu->identity.features & meta->feature)) > > continue; > > > > - if (meta->nr_domains < (index - offset)) { > > + if ((meta->nr_domains - 1) < (index - offset)) { > > While the logic is correct, I find this quite hard to read. A more > idiomatic way to write this (which is much easier to grok when reading > the code IMHO) would be: > > if (index - offset >= meta->nr_domains) > > If you agree, please send a v2 of this patch. > Works for me - will send a v2 in the evening.
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c index e6795bafcbb9..35f7171e779a 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain *pm_domain(const struct etnaviv_gpu *gpu, if (!(gpu->identity.features & meta->feature)) continue; - if (meta->nr_domains < (index - offset)) { + if ((meta->nr_domains - 1) < (index - offset)) { offset += meta->nr_domains; continue; }