Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fdtable: fix longstanding layering violations #75348

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fdtable: fix longstanding layering violations #75348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e5c2b9
kernel: types: add k_off_t and k_ssize_t types
Jul 6, 2024
1dfe21a
manifest: optional: update sof to use k_ssize_t
Jul 9, 2024
641a504
fdtable: remove inappropriate use of POSIX off_t and ssize_t
Jul 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 1 addition & 1 deletion drivers/bluetooth/hci/userchan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ static void rx_thread(void *p1, void *p2, void *p3)
struct net_buf *buf;
size_t buf_tailroom;
size_t buf_add_len;
ssize_t len;
k_ssize_t len;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really correct? This driver is for native_sim, and some 10 lines later the return value from the host libc's read() is assigned to it.

Copy link
Member Author

@cfriedt cfriedt Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definitions should be identical (as far as the underlying type goes). Otherwise, there would be warnings / errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't doubt that. I was mainly wondering where a native_sim driver like this conceptually sits in the software stack. It's calling the host OS APIs (like read()) so in that sense it could be seen to be above POSIX (in which case ssize_t would be more appropriate), however the driver also accesses Zephyr kernel APIs. Anyway, this isn't necessarily that critical (as you say, the types should be the same), which is why I didn't want to block this PR by requesting changes, but I do think it would be good to understand where native_sim driver code sits in the software stack.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's kind of a recursion. I would still say those drivers sit below (Zephyr's) POSIX API.

The namespace issue is kind of an unfortunate side-effect (not without solutions, ofc)

const uint8_t *frame_start = frame;

if (!uc_ready(uc->fd)) {
Expand Down
23 changes: 12 additions & 11 deletions drivers/disk/flashdisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ struct flashdisk_data {
struct disk_info info;
struct k_mutex lock;
const unsigned int area_id;
const off_t offset;
const k_off_t offset;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether using off_t or k_off_t is correct here.
I guess that the off_t has been used here because it may have larger range than size_t, but using signed int here is pointless, same with cached_addr. Note also that using 64bit values here, just to serve device that is >2GB and <=4GB, means that upper 4 bytes of the off_t will never be used.

I think that we need separate types for storage/external devices, with long addresses, independent from whatever kernel is running on.

Copy link
Member Author

@cfriedt cfriedt Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k_off_t (and off_t) are specifically for describing sizes and offsets within storage devices.

Using k_off_t or some other unsigned integer is fine with me, but this PR is specifically addressing the layering violation.

It might be better to retype this field in a separate PR.

uint8_t *const cache;
const size_t cache_size;
const size_t size;
const size_t sector_size;
size_t page_size;
off_t cached_addr;
k_off_t cached_addr;
bool cache_valid;
bool cache_dirty;
bool erase_required;
Expand Down Expand Up @@ -89,7 +89,7 @@ static int flashdisk_init_runtime(struct flashdisk_data *ctx,
{
int rc;
struct flash_pages_info page;
off_t offset;
k_off_t offset;

flashdisk_probe_erase(ctx);

Expand Down Expand Up @@ -126,7 +126,8 @@ static int flashdisk_init_runtime(struct flashdisk_data *ctx,
while (offset < ctx->offset + ctx->size) {
rc = flash_get_page_info_by_offs(ctx->info.dev, offset, &page);
if (rc < 0) {
LOG_ERR("Error %d getting page info at offset %lx", rc, offset);
LOG_ERR("Error %d getting page info at offset %lx", rc,
(long)offset);
return rc;
}
if (page.size != ctx->page_size) {
Expand Down Expand Up @@ -200,7 +201,7 @@ static int disk_flash_access_read(struct disk_info *disk, uint8_t *buff,
uint32_t start_sector, uint32_t sector_count)
{
struct flashdisk_data *ctx;
off_t fl_addr;
k_off_t fl_addr;
uint32_t remaining;
uint32_t offset;
uint32_t len;
Expand Down Expand Up @@ -272,7 +273,7 @@ static int flashdisk_cache_commit(struct flashdisk_data *ctx)
return 0;
}

static int flashdisk_cache_load(struct flashdisk_data *ctx, off_t fl_addr)
static int flashdisk_cache_load(struct flashdisk_data *ctx, k_off_t fl_addr)
{
int rc;

Expand Down Expand Up @@ -308,11 +309,11 @@ static int flashdisk_cache_load(struct flashdisk_data *ctx, off_t fl_addr)
/* input size is either less or equal to a block size (ctx->page_size)
* and write data never spans across adjacent blocks.
*/
static int flashdisk_cache_write(struct flashdisk_data *ctx, off_t start_addr,
uint32_t size, const void *buff)
static int flashdisk_cache_write(struct flashdisk_data *ctx, k_off_t start_addr, uint32_t size,
const void *buff)
{
int rc;
off_t fl_addr;
k_off_t fl_addr;
uint32_t offset;

/* adjust offset if starting address is not erase-aligned address */
Expand Down Expand Up @@ -347,7 +348,7 @@ static int disk_flash_access_write(struct disk_info *disk, const uint8_t *buff,
uint32_t start_sector, uint32_t sector_count)
{
struct flashdisk_data *ctx;
off_t fl_addr;
k_off_t fl_addr;
uint32_t remaining;
uint32_t size;
int rc = 0;
Expand All @@ -369,7 +370,7 @@ static int disk_flash_access_write(struct disk_info *disk, const uint8_t *buff,

/* check if start address is erased-aligned address */
if (fl_addr & (ctx->page_size - 1)) {
off_t block_bnd;
k_off_t block_bnd;

/* not aligned */
/* check if the size goes over flash block boundary */
Expand Down
2 changes: 1 addition & 1 deletion drivers/display/display_hx8394.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ const uint8_t hx8394_cmd3[] = {0xC6U, 0xEDU};

const uint8_t tear_config[] = {HX8394_SET_TEAR, HX8394_TEAR_VBLANK};

static ssize_t hx8394_mipi_tx(const struct device *mipi_dev, uint8_t channel,
static k_ssize_t hx8394_mipi_tx(const struct device *mipi_dev, uint8_t channel,
const void *buf, size_t len)
{
/* Send MIPI transfers using low power mode */
Expand Down
31 changes: 10 additions & 21 deletions drivers/eeprom/eeprom_at2x.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ static inline int eeprom_at2x_write_enable(const struct device *dev)
}
#endif /* ANY_INST_HAS_WP_GPIOS */

static int eeprom_at2x_read(const struct device *dev, off_t offset, void *buf,
size_t len)
static int eeprom_at2x_read(const struct device *dev, k_off_t offset, void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
struct eeprom_at2x_data *data = dev->data;
Expand Down Expand Up @@ -124,13 +123,11 @@ static int eeprom_at2x_read(const struct device *dev, off_t offset, void *buf,
return 0;
}

static size_t eeprom_at2x_limit_write_count(const struct device *dev,
off_t offset,
size_t len)
static size_t eeprom_at2x_limit_write_count(const struct device *dev, k_off_t offset, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
size_t count = len;
off_t page_boundary;
k_off_t page_boundary;

/* We can at most write one page at a time */
if (count > config->pagesize) {
Expand All @@ -146,9 +143,7 @@ static size_t eeprom_at2x_limit_write_count(const struct device *dev,
return count;
}

static int eeprom_at2x_write(const struct device *dev, off_t offset,
const void *buf,
size_t len)
static int eeprom_at2x_write(const struct device *dev, k_off_t offset, const void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
struct eeprom_at2x_data *data = dev->data;
Expand Down Expand Up @@ -233,8 +228,7 @@ static bool eeprom_at24_bus_is_ready(const struct device *dev)
* but also to address higher part of eeprom for chips
* with more than 2^(addr_width) adressable word.
*/
static uint16_t eeprom_at24_translate_offset(const struct device *dev,
off_t *offset)
static uint16_t eeprom_at24_translate_offset(const struct device *dev, k_off_t *offset)
{
const struct eeprom_at2x_config *config = dev->config;

Expand All @@ -244,8 +238,7 @@ static uint16_t eeprom_at24_translate_offset(const struct device *dev,
return config->bus.i2c.addr + addr_incr;
}

static size_t eeprom_at24_adjust_read_count(const struct device *dev,
off_t offset, size_t len)
static size_t eeprom_at24_adjust_read_count(const struct device *dev, k_off_t offset, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
const size_t remainder = BIT(config->addr_width) - offset;
Expand All @@ -257,8 +250,7 @@ static size_t eeprom_at24_adjust_read_count(const struct device *dev,
return len;
}

static int eeprom_at24_read(const struct device *dev, off_t offset, void *buf,
size_t len)
static int eeprom_at24_read(const struct device *dev, k_off_t offset, void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
int64_t timeout;
Expand Down Expand Up @@ -299,8 +291,7 @@ static int eeprom_at24_read(const struct device *dev, off_t offset, void *buf,
return len;
}

static int eeprom_at24_write(const struct device *dev, off_t offset,
const void *buf, size_t len)
static int eeprom_at24_write(const struct device *dev, k_off_t offset, const void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
int count = eeprom_at2x_limit_write_count(dev, offset, len);
Expand Down Expand Up @@ -415,8 +406,7 @@ static int eeprom_at25_wait_for_idle(const struct device *dev)
return -EBUSY;
}

static int eeprom_at25_read(const struct device *dev, off_t offset, void *buf,
size_t len)
static int eeprom_at25_read(const struct device *dev, k_off_t offset, void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
struct eeprom_at2x_data *data = dev->data;
Expand Down Expand Up @@ -502,8 +492,7 @@ static int eeprom_at25_wren(const struct device *dev)
return spi_write_dt(&config->bus.spi, &tx);
}

static int eeprom_at25_write(const struct device *dev, off_t offset,
const void *buf, size_t len)
static int eeprom_at25_write(const struct device *dev, k_off_t offset, const void *buf, size_t len)
{
const struct eeprom_at2x_config *config = dev->config;
int count = eeprom_at2x_limit_write_count(dev, offset, len);
Expand Down
48 changes: 24 additions & 24 deletions drivers/eeprom/eeprom_emulator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ struct eeprom_emu_config {
*/
size_t page_size;
/* Offset of the flash partition used to emulate the EEPROM */
off_t flash_offset;
k_off_t flash_offset;
/* Size of the flash partition to emulate the EEPROM */
size_t flash_size;
/* Delay the erase of EEPROM pages until the complete partition is used.
Expand All @@ -89,24 +89,24 @@ struct eeprom_emu_config {

struct eeprom_emu_data {
/* Offset in current (EEPROM) page where next change is written */
off_t write_offset;
k_off_t write_offset;
/* Offset of the current (EEPROM) page */
off_t page_offset;
k_off_t page_offset;
struct k_mutex lock;
};

/* read/write context */
struct eeprom_emu_ctx {
const void *data; /* pointer to data */
const size_t len; /* data length */
const off_t address; /* eeprom address */
const k_off_t address; /* eeprom address */
size_t rlen; /* data remaining (unprocessed) length */
};

/*
* basic flash read, only used with offset aligned to flash write block size
*/
static inline int eeprom_emu_flash_read(const struct device *dev, off_t offset,
static inline int eeprom_emu_flash_read(const struct device *dev, k_off_t offset,
uint8_t *blk, size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand All @@ -118,7 +118,7 @@ static inline int eeprom_emu_flash_read(const struct device *dev, off_t offset,
/*
* basic flash write, only used with offset aligned to flash write block size
*/
static inline int eeprom_emu_flash_write(const struct device *dev, off_t offset,
static inline int eeprom_emu_flash_write(const struct device *dev, k_off_t offset,
const uint8_t *blk, size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand All @@ -133,7 +133,7 @@ static inline int eeprom_emu_flash_write(const struct device *dev, off_t offset,
* basic flash erase, only used with offset aligned to flash page and len a
* multiple of the flash page size
*/
static inline int eeprom_emu_flash_erase(const struct device *dev, off_t offset,
static inline int eeprom_emu_flash_erase(const struct device *dev, k_off_t offset,
size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand All @@ -147,7 +147,7 @@ static inline int eeprom_emu_flash_erase(const struct device *dev, off_t offset,
/*
* eeprom_emu_page_invalidate: invalidate a page by writing all zeros at the end
*/
static int eeprom_emu_page_invalidate(const struct device *dev, off_t offset)
static int eeprom_emu_page_invalidate(const struct device *dev, k_off_t offset)
{
const struct eeprom_emu_config *dev_config = dev->config;
uint8_t buf[dev_config->flash_cbs];
Expand Down Expand Up @@ -227,13 +227,13 @@ static int eeprom_emu_is_word_used(const struct device *dev, const uint8_t *blk)
* eeprom_emu_word_read: read basic word (cbs byte of data) item from
* address directly from flash.
*/
static int eeprom_emu_word_read(const struct device *dev, off_t address,
static int eeprom_emu_word_read(const struct device *dev, k_off_t address,
uint8_t *data)
{
const struct eeprom_emu_config *dev_config = dev->config;
const struct eeprom_emu_data *dev_data = dev->data;
uint8_t buf[dev_config->flash_cbs];
off_t direct_address;
k_off_t direct_address;
int rc;

direct_address = dev_data->page_offset + address;
Expand All @@ -245,7 +245,7 @@ static int eeprom_emu_word_read(const struct device *dev, off_t address,
}

/* Process changes written to flash */
off_t offset, ch_address;
k_off_t offset, ch_address;
bool mc1 = false, mc2 = false;

offset = dev_data->write_offset;
Expand Down Expand Up @@ -280,10 +280,10 @@ static int eeprom_emu_flash_get(const struct device *dev,
struct eeprom_emu_ctx *ctx)
{
const struct eeprom_emu_config *dev_config = dev->config;
off_t address = ctx->address + ctx->len - ctx->rlen;
k_off_t address = ctx->address + ctx->len - ctx->rlen;
uint8_t *data8 = (uint8_t *)(ctx->data);
uint8_t buf[dev_config->flash_cbs];
const off_t addr_jmp = address & (sizeof(buf) - 1);
const k_off_t addr_jmp = address & (sizeof(buf) - 1);
size_t len;
int rc;

Expand All @@ -310,7 +310,7 @@ static int eeprom_emu_compactor(const struct device *dev,
{
const struct eeprom_emu_config *dev_config = dev->config;
struct eeprom_emu_data *dev_data = dev->data;
off_t next_page_offset;
k_off_t next_page_offset;
int rc = 0;

LOG_DBG("Compactor called for page at [0x%tx]",
Expand Down Expand Up @@ -347,7 +347,7 @@ static int eeprom_emu_compactor(const struct device *dev,

ctx->rlen = 0;
} else {
off_t rd_offset = 0;
k_off_t rd_offset = 0;
uint8_t buf[dev_config->flash_cbs];

/* reset the context if available */
Expand All @@ -367,7 +367,7 @@ static int eeprom_emu_compactor(const struct device *dev,
(rd_offset > (ctx->address - sizeof(buf)))) {
/* overwrite buf data with context data */
uint8_t *data8 = (uint8_t *)(ctx->data);
off_t address, addr_jmp;
k_off_t address, addr_jmp;
size_t len;

address = ctx->address + ctx->len - ctx->rlen;
Expand Down Expand Up @@ -412,14 +412,14 @@ static int eeprom_emu_compactor(const struct device *dev,
/*
* eeprom_emu_word_write: write basic word (cbs bytes of data) item to address,
*/
static int eeprom_emu_word_write(const struct device *dev, off_t address,
static int eeprom_emu_word_write(const struct device *dev, k_off_t address,
const uint8_t *data,
struct eeprom_emu_ctx *ctx)
{
const struct eeprom_emu_config *dev_config = dev->config;
struct eeprom_emu_data *dev_data = dev->data;
uint8_t buf[dev_config->flash_cbs], tmp[dev_config->flash_cbs];
off_t direct_address, wraddr;
k_off_t direct_address, wraddr;
int rc;

direct_address = dev_data->page_offset + address;
Expand Down Expand Up @@ -482,10 +482,10 @@ static int eeprom_emu_flash_set(const struct device *dev,
struct eeprom_emu_ctx *ctx)
{
const struct eeprom_emu_config *dev_config = dev->config;
off_t address = ctx->address + ctx->len - ctx->rlen;
k_off_t address = ctx->address + ctx->len - ctx->rlen;
uint8_t *data8 = (uint8_t *)(ctx->data);
uint8_t buf[dev_config->flash_cbs];
const off_t addr_jmp = address & (sizeof(buf) - 1);
const k_off_t addr_jmp = address & (sizeof(buf) - 1);
size_t len;
int rc;

Expand All @@ -509,7 +509,7 @@ static int eeprom_emu_flash_set(const struct device *dev,
return rc;
}

static int eeprom_emu_range_is_valid(const struct device *dev, off_t address,
static int eeprom_emu_range_is_valid(const struct device *dev, k_off_t address,
size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand All @@ -521,7 +521,7 @@ static int eeprom_emu_range_is_valid(const struct device *dev, off_t address,
return 0;
}

static int eeprom_emu_read(const struct device *dev, off_t address, void *data,
static int eeprom_emu_read(const struct device *dev, k_off_t address, void *data,
size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand Down Expand Up @@ -572,7 +572,7 @@ static int eeprom_emu_read(const struct device *dev, off_t address, void *data,
return rc;
}

static int eeprom_emu_write(const struct device *dev, off_t address,
static int eeprom_emu_write(const struct device *dev, k_off_t address,
const void *data, size_t len)
{
const struct eeprom_emu_config *dev_config = dev->config;
Expand Down Expand Up @@ -640,7 +640,7 @@ static int eeprom_emu_init(const struct device *dev)
{
const struct eeprom_emu_config *dev_config = dev->config;
struct eeprom_emu_data *dev_data = dev->data;
off_t offset;
k_off_t offset;
uint8_t buf[dev_config->flash_cbs];
int rc = 0;

Expand Down
Loading
Loading