Gustavo A. R. Silva
2025-Apr-07 23:35 UTC
[PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings
[..]>>>> - struct { >>>> - struct nvif_chan_v0 chan; >>>> - char name[TASK_COMM_LEN+16]; >>>> - } args; >>>> + DEFINE_RAW_FLEX(struct nvif_chan_v0, args, name, TASK_COMM_LEN + 16); >>>> struct nvif_device *device = &cli->device; >>>> struct nouveau_channel *chan; >>>> const u64 plength = 0x10000; >>>> @@ -298,28 +295,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, >>>> return ret; >>>> /* create channel object */ >>>> - args.chan.version = 0; >>>> - args.chan.namelen = sizeof(args.name); >>>> - args.chan.runlist = __ffs64(runm); >>>> - args.chan.runq = 0; >>>> - args.chan.priv = priv; >>>> - args.chan.devm = BIT(0); >>>> + args->version = 0; >>>> + args->namelen = __struct_size(args) - sizeof(*args); >>> >>> Does __struct_size(args->name) work here (and later)? >> >> Why not? > > Uhm, I'm genuinely curious. I *think* it will work, but because it's > within the struct, not outside of it, I'm unclear if it'll DTRT for > finding the size (since __builtin_object_size() can be touchy). > >> I mean, this should be equivalent to `TASK_COMM_LEN+16`, I could >> use the latter if people prefer it (see my comments below). > > Right, it should be the same. I think __struct_size(args->name) would be > much more readable ... if it works. :)OK, I'll double check this. [..]>>>> @@ -367,17 +365,17 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart) >>>> return ret; >>>> if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) { >>>> - struct { >>>> - struct nvif_event_v0 base; >>>> - struct nvif_chan_event_v0 host; >>>> - } args; >>>> + DEFINE_RAW_FLEX(struct nvif_event_v0, args, data, >>>> + sizeof(struct nvif_chan_event_v0)); >>>> + struct nvif_chan_event_v0 *host >>>> + (struct nvif_chan_event_v0 *)args->data; >>>> - args.host.version = 0; >>>> - args.host.type = NVIF_CHAN_EVENT_V0_KILLED; >>>> + host->version = 0; >>>> + host->type = NVIF_CHAN_EVENT_V0_KILLED; >>>> ret = nvif_event_ctor(&chan->user, "abi16ChanKilled", chan->chid, >>>> nouveau_channel_killed, false, >>>> - &args.base, sizeof(args), &chan->kill); >>>> + args, __struct_size(args), &chan->kill); >>>> if (ret == 0) >>>> ret = nvif_event_allow(&chan->kill); >>>> if (ret) { >>>> @@ -520,46 +518,45 @@ nouveau_channels_fini(struct nouveau_drm *drm) >>>> int >>>> nouveau_channels_init(struct nouveau_drm *drm) >>>> { >>>> - struct { >>>> - struct nv_device_info_v1 m; >>>> - struct { >>>> - struct nv_device_info_v1_data channels; >>>> - struct nv_device_info_v1_data runlists; >>>> - } v; >>>> - } args = { >>>> - .m.version = 1, >>>> - .m.count = sizeof(args.v) / sizeof(args.v.channels), >>> >>> sizeof(args.v) == sizeof(struct nv_device_info_v1_data) * 2 >>> >>> and sizeof(args.v.channels) == sizeof(struct nv_device_info_v1_data). >>> >>> Isn't this just "2"? i.e. isn't struct nv_device_info_v1::count the >>> counted_by for struct nv_device_info_v1::data? >> >> Yes, it's just `2`. However, I didn't want to explicitly use the magic >> number, in case people don't like it, as in other similar patches (in >> other subsystems). >> >> But, yeah, it's `2`. :) > > Okay. So if "count" is set up as a counted_by, the assignment will > happen automatically (in DEFINE_FLEX -- no longer "RAW").I really don't want to condition -Wflex-array-member-not-at-end patches on counted_by patches, for now. Thanks -- Gustavo
Kees Cook
2025-Apr-08 23:40 UTC
[PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings
On Mon, Apr 07, 2025 at 05:35:47PM -0600, Gustavo A. R. Silva wrote:> [..] > > > > > > - struct { > > > > > - struct nvif_chan_v0 chan; > > > > > - char name[TASK_COMM_LEN+16]; > > > > > - } args; > > > > > + DEFINE_RAW_FLEX(struct nvif_chan_v0, args, name, TASK_COMM_LEN + 16); > > > > > struct nvif_device *device = &cli->device; > > > > > struct nouveau_channel *chan; > > > > > const u64 plength = 0x10000; > > > > > @@ -298,28 +295,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, > > > > > return ret; > > > > > /* create channel object */ > > > > > - args.chan.version = 0; > > > > > - args.chan.namelen = sizeof(args.name); > > > > > - args.chan.runlist = __ffs64(runm); > > > > > - args.chan.runq = 0; > > > > > - args.chan.priv = priv; > > > > > - args.chan.devm = BIT(0); > > > > > + args->version = 0; > > > > > + args->namelen = __struct_size(args) - sizeof(*args); > > > > > > > > Does __struct_size(args->name) work here (and later)? > > > > > > Why not? > > > > Uhm, I'm genuinely curious. I *think* it will work, but because it's > > within the struct, not outside of it, I'm unclear if it'll DTRT for > > finding the size (since __builtin_object_size() can be touchy). > > > > > I mean, this should be equivalent to `TASK_COMM_LEN+16`, I could > > > use the latter if people prefer it (see my comments below). > > > > Right, it should be the same. I think __struct_size(args->name) would be > > much more readable ... if it works. :) > > OK, I'll double check this.Ah-ha, yes, I'm already testing this with KUnit: struct bar { int a; u32 counter; s16 array[]; }; ... DEFINE_RAW_FLEX(struct bar, two, array, 2); ... KUNIT_EXPECT_EQ(test, sizeof(*two), sizeof(struct bar)); KUNIT_EXPECT_EQ(test, __struct_size(two), sizeof(struct bar) + 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __member_size(two), sizeof(struct bar) + 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __struct_size(two->array), 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __member_size(two->array), 2 * sizeof(s16));> I really don't want to condition -Wflex-array-member-not-at-end patches > on counted_by patches, for now.Fair enough. :) One thing at a time is wise! -- Kees Cook