linux-kernel - Re: [PATCH 04/12] drm/nouveau/bar/nvc0: support chips without BAR3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAVeFuKUu-oN-sMEcvcZVTuhcz9dZVauvca7T1_iN5=qkU7Zcg@mail.gmail.com>
Date:	Wed, 2 Apr 2014 22:47:28 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Alexandre Courbot <acourbot@...dia.com>,
	Ben Skeggs <bskeggs@...hat.com>,
	"nouveau@...ts.freedesktop.org" <nouveau@...ts.freedesktop.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 04/12] drm/nouveau/bar/nvc0: support chips without BAR3

On Tue, Mar 25, 2014 at 7:10 AM, Thierry Reding
<thierry.reding@...il.com> wrote:
> On Mon, Mar 24, 2014 at 05:42:26PM +0900, Alexandre Courbot wrote:
> [...]
>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/bar/nvc0.c b/drivers/gpu/drm/nouveau/core/subdev/bar/nvc0.c
> [...]
>>  static int
>> -nvc0_bar_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
>> -           struct nouveau_oclass *oclass, void *data, u32 size,
>> -           struct nouveau_object **pobject)
>> +nvc0_bar_init_vm(struct nvc0_bar_priv *priv, int nr, int bar)
>>  {
> [...]
>> -     /* BAR3 */
>>       ret = nouveau_gpuobj_new(nv_object(priv), NULL, 0x1000, 0, 0,
>> -                             &priv->bar[0].mem);
>> -     mem = priv->bar[0].mem;
>> +                             &priv->bar[nr].mem);
>> +     mem = priv->bar[nr].mem;
>>       if (ret)
>>               return ret;
>>
>>       ret = nouveau_gpuobj_new(nv_object(priv), NULL, 0x8000, 0, 0,
>> -                             &priv->bar[0].pgd);
>> +                             &priv->bar[nr].pgd);
>>       if (ret)
>>               return ret;
> [...]
>> +static int
>> +nvc0_bar_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
>> +           struct nouveau_oclass *oclass, void *data, u32 size,
>> +           struct nouveau_object **pobject)
>> +{
> [...]
>> +     /* BAR3 */
>> +     if (has_bar3) {
>> +             ret = nvc0_bar_init_vm(priv, 0, 3);
> [...]
>> +     /* BAR1 */
>> +     ret = nvc0_bar_init_vm(priv, 1, 1);
>>       if (ret)
>>               return ret;
>
> The calls to nvc0_bar_init_vm() are somewhat confusing in my opinion. It
> is hard to see from the invocation what these numbers mean and therefore
> distinguish which parameter is which.
>
> Perhaps a slightly more readable way would be to pass in a pointer to a
> structure as second parameter instead of the index into an array. So
> it'd look somewhat like this:
>
>         if (has_bar3) {
>                 ret = nvc0_bar_init_vm(priv, &priv->bar[0], 3);
>                 ...
>         }
>         ...
>         ret = nvc0_bar_init_vm(priv, &priv->bar[1], 1);
>         ...
>
> Unfortunately that would require a new type to be created for the bar[]
> structures, so it'd be slightly more intrusive.

These types are local to nvc0.c anyway, so I don't think it would
hurt. And you are right that the code would become more readable as a
result, passing array indexes as arguments is not a common practice
(and should not be).

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