* Re: [PATCH] firewire: core: use struct_size and flex_array_size in ioctl_add_descriptor
2025-09-16 12:21 [PATCH] firewire: core: use struct_size and flex_array_size in ioctl_add_descriptor Thorsten Blum
@ 2025-09-17 15:40 ` Takashi Sakamoto
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Sakamoto @ 2025-09-17 15:40 UTC (permalink / raw)
To: Thorsten Blum; +Cc: linux1394-devel, linux-kernel
Hi,
On Tue, Sep 16, 2025 at 02:21:45PM +0200, Thorsten Blum wrote:
> Use struct_size() to determine the memory needed for a new 'struct
> descriptor_resource' and flex_array_size() to calculate the number of
> bytes to copy from userspace. This removes the hardcoded size (4 bytes)
> for the 'u32 data[]' entries.
>
> No functional changes intended.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> drivers/firewire/core-cdev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Applied to for-next branch, since it works as long as I tested with the
following userspace appliaction:
```
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <unistd.h>
#include <sys/epoll.h>
#include <string.h>
#include <errno.h>
#include <linux/firewire-cdev.h>
#include <linux/firewire-constants.h>
// From include/linux/firewire.h
#define CSR_CONFIG_ROM 0x400
#define CSR_CONFIG_ROM_END 0x800
#define CSR_LEAF 0x80
#define CSR_DIRECTORY 0xc0
#define CSR_DESCRIPTOR 0x01
#define CSR_VENDOR 0x03
#define CSR_UNIT 0x11
#define CSR_SPECIFIER_ID 0x12
#define CSR_VERSION 0x13
#define CSR_MODEL 0x17
// See https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/commit/?id=d71e6a11737f
#define ROOT_VENDOR_ID_OLD 0xd00d1e
#define ROOT_VENDOR_ID 0x001f11
#define ROOT_MODEL_ID 0x023901
// Unit directory for AV/C protocol.
#define AM_UNIT_SPEC_1394TA 0x00a02d
#define AM_UNIT_VERSION_AVC 0x010001
#define AM_UNIT_MODEL_ID 0x023903
#define AM_UNIT_NAME_0 0x4c696e75 // Linu
#define AM_UNIT_NAME_1 0x7820414c // x AL
#define AM_UNIT_NAME_2 0x53410000 // SA..
static int add_descriptor(int fd, uint32_t *handle_value)
{
uint32_t directory[] = {
0x00040000, // Unit directory consists of below 4 quads
(CSR_SPECIFIER_ID << 24) | AM_UNIT_SPEC_1394TA,
(CSR_VERSION << 24) | AM_UNIT_VERSION_AVC,
(CSR_MODEL << 24) | AM_UNIT_MODEL_ID,
((CSR_LEAF | CSR_DESCRIPTOR) << 24) | 0x00000001, // Begin at next.
0x00050000, // Text leaf consists of below 5 quads.
0x00000000,
0x00000000,
AM_UNIT_NAME_0,
AM_UNIT_NAME_1,
AM_UNIT_NAME_2,
};
struct fw_cdev_add_descriptor data = {
.immediate = 0x0c0083c0,
.key = (CSR_DIRECTORY | CSR_UNIT) << 24,
.data = (uint64_t)directory,
.length = sizeof(directory) / sizeof(uint32_t),
};
if (ioctl(fd, FW_CDEV_IOC_ADD_DESCRIPTOR, &data)) {
printf("ioctl(FW_CDEV_IOC_ADD_DESCRIPTOR): %s\n", strerror(errno));
return -errno;
}
*handle_value = data.handle;
return 0;
}
static void remove_descriptor(int fd, uint32_t handle_value)
{
struct fw_cdev_remove_descriptor data = {
.handle = handle_value,
};
if (ioctl(fd, FW_CDEV_IOC_REMOVE_DESCRIPTOR, &data))
printf("ioctl(FW_CDEV_IOC_REMOVE_DESCRIPTOR): %s\n", strerror(errno));
}
static int retrieve_config_rom(int fd, uint32_t *content, uint32_t *length,
struct fw_cdev_event_bus_reset *bus_reset)
{
struct fw_cdev_get_info info = {
.version = 6,
.rom_length = *length,
.rom = (uint64_t)content,
.bus_reset = (uint64_t)bus_reset,
};
memset(bus_reset, 0, sizeof(*bus_reset));
if (ioctl(fd, FW_CDEV_IOC_GET_INFO, &info)) {
printf("ioctl(FW_CDEV_IOC_GET_INFO): %s\n", strerror(errno));
return -errno;
}
*length = info.rom_length;
return 0;
}
static int wait_event(int epfd, int fd, union fw_cdev_event *event)
{
struct epoll_event ep_event = {
.events = EPOLLIN,
};
int nfds;
ssize_t length;
memset(event, 0, sizeof(*event));
if (epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ep_event) < 0) {
printf("epoll_ctl(): %s\n", strerror(errno));
return -errno;
}
nfds = epoll_wait(epfd, &ep_event, 1, 500);
if (nfds < 0) {
printf("epoll_wait(): %s\n", strerror(errno));
return -errno;
}
if (nfds == 0) {
printf("epoll_wait(): %s\n", strerror(ETIMEDOUT));
return -ETIMEDOUT;
}
epoll_ctl(epfd, EPOLL_CTL_DEL, fd, NULL);
length = read(fd, event, sizeof(*event));
if (length < 0) {
printf("read: %s\n", strerror(errno));
return -errno;
}
return 0;
}
int main()
{
struct {
unsigned int generation;
uint32_t content[CSR_CONFIG_ROM_END - CSR_CONFIG_ROM];
uint32_t length;
} images[3];
int fd, epfd;
uint32_t handle_value;
union fw_cdev_event event;
size_t maximum_quadlet_count;
int i;
int err;
for (i = 0; i < sizeof(images) / sizeof(*images); ++i) {
memset(images[i].content, 0, sizeof(images[i].content));
images[i].length = sizeof(images[i].content);
}
epfd = epoll_create1(0);
if (epfd < 0) {
printf("epoll_create1(): %s\n", strerror(errno));
return EXIT_FAILURE;
}
fd = open("/dev/fw0", O_RDONLY);
if (fd < 0) {
printf("open(): %s\n", strerror(errno));
close(epfd);
return EXIT_FAILURE;
}
//
// Initial state.
//
err = retrieve_config_rom(fd, images[0].content, &images[0].length, &event.bus_reset);
if (err < 0) {
printf("Fail to retrieve config rom take 0\n");
goto end;
}
images[0].generation = event.bus_reset.generation;
//
// Add descriptor.
//
err = add_descriptor(fd, &handle_value);
if (err < 0)
goto end;
err = wait_event(epfd, fd, &event);
if (err < 0)
goto end;
if (event.common.type != FW_CDEV_EVENT_BUS_RESET) {
printf("Unexpected event type: %d\n", event.common.type);
goto end;
}
err = retrieve_config_rom(fd, images[1].content, &images[1].length, &event.bus_reset);
if (err < 0) {
printf("Fail to retrieve config rom take 1\n");
goto end;
}
images[1].generation = event.bus_reset.generation;
// Requires delay for 2s after last reset per IEEE 1394 clause 8.2.1.
sleep(2);
//
// Clean up the descriptor.
//
remove_descriptor(fd, handle_value);
err = wait_event(epfd, fd, &event);
if (err < 0)
goto end;
err = retrieve_config_rom(fd, images[2].content, &images[2].length, &event.bus_reset);
if (err < 0) {
printf("Fail to retrieve config rom take 2\n");
goto end;
}
images[2].generation = event.bus_reset.generation;
//
// Check these contents.
//
maximum_quadlet_count = 0;
for (i = 0; i < sizeof(images) / sizeof(*images); ++i) {
size_t quadlet_count = images[i].length / sizeof(uint32_t);
if (quadlet_count > maximum_quadlet_count)
maximum_quadlet_count = quadlet_count;
}
printf("Gen ");
for (i = 0; i < sizeof(images) / sizeof(*images); ++i) {
printf("%11u", images[i].generation);
}
printf("\n");
for (i = 0; i < maximum_quadlet_count; ++i) {
int j;
printf("[%2d] ", i);
for (j = 0; j < sizeof(images) / sizeof(*images); ++j) {
if (i < images[j].length / sizeof(uint32_t))
printf("0x%08x ", images[j].content[i]);
else
printf(" - ");
}
printf("\n");
}
end:
close(fd);
close(epfd);
return EXIT_SUCCESS;
}
```
```
$ gcc -Wall -o test test.c
$ sudo ./test
Gen 3 4 5
[ 0] 0x0404f693 0x0404a56d 0x0404516f
[ 1] 0x31333934 0x31333934 0x31333934
[ 2] 0xf000b243 0xf000b253 0xf000b263
[ 3] 0x08002851 0x08002851 0x08002851
[ 4] 0x0100014a 0x0100014a 0x0100014a
[ 5] 0x00051b70 0x0007eca5 0x00051b70
[ 6] 0x0c0083c0 0x0c0083c0 0x0c0083c0
[ 7] 0x03001f11 0x03001f11 0x03001f11
[ 8] 0x81000003 0x81000005 0x81000003
[ 9] 0x17023901 0x17023901 0x17023901
[10] 0x81000008 0x8100000a 0x81000008
[11] 0x00064cb7 0x0c0083c0 0x00064cb7
[12] 0x00000000 0xd100000c 0x00000000
[13] 0x00000000 0x00064cb7 0x00000000
[14] 0x4c696e75 0x00000000 0x4c696e75
[15] 0x78204669 0x00000000 0x78204669
[16] 0x72657769 0x4c696e75 0x72657769
[17] 0x72650000 0x78204669 0x72650000
[18] 0x0003ff1c 0x72657769 0x0003ff1c
[19] 0x00000000 0x72650000 0x00000000
[20] 0x00000000 0x0003ff1c 0x00000000
[21] 0x4a756a75 0x00000000 0x4a756a75
[22] - 0x00000000 -
[23] - 0x4a756a75 -
[24] - 0x000466d5 -
[25] - 0x1200a02d -
[26] - 0x13010001 -
[27] - 0x17023903 -
[28] - 0x81000001 -
[29] - 0x00054009 -
[30] - 0x00000000 -
[31] - 0x00000000 -
[32] - 0x4c696e75 -
[33] - 0x7820414c -
[34] - 0x53410000 -
```
Thanks
Takashi Sakamoto
^ permalink raw reply [flat|nested] 2+ messages in thread