You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/28 18:25:40 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #4248: mtd: Add MTDIOC_FIRSTBLOCK for retrieving an MTD partition first block
gustavonihei opened a new pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248
## Summary
This PR intends to add a new IOCTL command for retrieving the first block of an MTD partition.
## Impact
New feature, should have no impact to existing MTD devices or applications.
## Testing
`esp32-wrover-kit` with WIP development (see #4233)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682923925
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant user solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r685406503
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,12 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_PARTINFO _MTDIOC(0x000b) /* IN: Pointer to write-able struct
Review comment:
Should we add BIOC_PARTINFO in this PR? I would prefer the userspace program use BIOC_PARTINFO instead MTDIOC_PARTINFO since this make the program work with both block and mtd device.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to divide the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the `MTDIOC_GEOMETRY`, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r683121868
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
@gustavonihei it's reasonable to split the big change into the small patch, your propose is fine to me. Thanks for your effort to provide a general solution.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
@gustavonihei it's reasonable to split a big change into the small patch, your propose is fine to me. Thanks for your effort to provide a general solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680116455
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
There has always been at least two methods for retrieving information from MTD:
- `ioctl` commands from the `MTDIOC_*`;
- `procfs`;
Currently neither approach provides the `firstblock` information.
So, consider an application that already owns the path to the MTD character device (via BCH and FTL) and already manipulates the MTD partition via file descriptors. So this is not one particular use case, it is a fairly common one.
Linux also provides this information via `ioctl` calls.
`procfs` requires `CONFIG_FS_PROCFS`, which is an optional feature. Adding `CONFIG_FS_PROCFS`, even after disabling all `procfs` entries except for the **partitions**, this imposes a footprint overhead of 2460 bytes to the binary file. There is also the runtime execution overhead of the two-way string conversion.
Do you really think it is worth paying the price for this coupling to `procfs`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682415221
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
It's good to follow Linux design, two question:
1. Should we also use MEMGETINFO and mtd_info_t too?
2. How about block driver which should have the similar requirement too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
uint32_t magic; /* File-system magic (0 for RAW) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
off_t startsector; /* Offset to the first block/section of the managed
* sub-region */
off_t endsector; /* Offset to the last block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r684634260
##########
File path: include/nuttx/fs/fs.h
##########
@@ -224,6 +224,20 @@ struct geometry
blksize_t geo_sectorsize; /* Size of one sector */
};
+struct partition_info_s
+{
+ uint32_t magic; /* File system magic, 0 for RAW
+ * (see <sys/statfs.h>) */
+ size_t length; /* Partition size in bytes */
Review comment:
redundanct info? can computed from numsectors and sectorsize.
##########
File path: include/nuttx/fs/fs.h
##########
@@ -224,6 +224,20 @@ struct geometry
blksize_t geo_sectorsize; /* Size of one sector */
};
+struct partition_info_s
+{
+ uint32_t magic; /* File system magic, 0 for RAW
+ * (see <sys/statfs.h>) */
+ size_t length; /* Partition size in bytes */
+ size_t numsectors; /* Number of sectors in the partition */
+ size_t sectorsize; /* Size in bytes of a single sector */
+ off_t startsector; /* Offset to the first section/block of the
+ * managed sub-region */
+ off_t endsector; /* Offset to the last section/block of the
+ * managed sub-region */
+ FAR const char *parent; /* Name of the parent node of the partition */
Review comment:
change to array, otherwise userspace can't access the kernel pointer
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,12 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_PARTINFO _MTDIOC(0x000b) /* IN: Pointer to write-able struct
Review comment:
change to FIOC_PARTINFO?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
uint32_t magic; /* File system magic, 0 for RAW
* (see <sys/statfs.h>) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
off_t startsector; /* Offset to the first section/block of the
* managed sub-region */
off_t endsector; /* Offset to the last section/block of the
* managed sub-region */
FAR const char *parent; /* Name of the parent node of the partition */
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r681949575
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Some more evidences to justify the `ioctl` path:
1) Linux MTD character device driver implements similar `ioctl` commands (**MEMGETREGIONINFO** and **MEMGETINFO**) for providing MTD info to the application:
https://github.com/torvalds/linux/blob/master/drivers/mtd/mtdchar.c#L684-L718
2) `mtd-utils` rely on **MEMGETINFO** for retrieving MTD partition information:
https://github.com/sigma-star/mtd-utils/blob/master/misc-utils/ftl_check.c#L80-L84
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679220521
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
is it start better than first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680089452
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It seems like a VERY odd technical decision to duplicate an existing interface purely to satisfy your personal preference. Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
It's sad to hear that you believe I am bringing personal motivations to this PR. My original intent was to just provide one single piece of data to the application (the `firstblock` info pro MTD partition), and I am dedicating my time to work with you after each round of discussions, with the sole purpose of trying to do what's best to the project.
So, please, I'd ask you to be a bit more reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires the a single information, don't you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679507386
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
The latter seems more reasonable, so that it would be possible to iterate the partition list, similar to what Linux has for MTD erase regions.
@xiaoxiang781216's suggestions for an `MTDIOC_PARTINFO` could also fit for when the caller already contains a direct reference to the partition (e.g. via a chardev path).
So, for an initial implementation, this could be the structure for a partition info:
```c
struct mtd_partition_info_s
{
size_t nblocks; /* Number of blocks in the partition */
off_t startblock; /* Offset to the first block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
For consistency, `part_ioctl` would then always forward the handling of `MTDIOC_GEOMETRY` to the parent MTD.
The application would now have the responsibility for calculating the partition size by invoking both `MTDIOC_GEOMETRY` (for `blocksize`) and `MTDIOC_PARTINFO` (for `nblocks`).
Is this approach reasonable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r683121868
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
@gustavonihei it's reasonable to split the big change into the small patch, your propose is fine to me. Thanks for your effort to provide a general solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r685413841
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,12 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_PARTINFO _MTDIOC(0x000b) /* IN: Pointer to write-able struct
Review comment:
Ok, I think we may already create the command definition in this PR.
I'll add it to the list of `ioctl` commands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679241032
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Rather, it would make more sense to provide more complete partition information as you get from a partition tool like fdisk.
Initially the idea of a new `MTDIOC_FIRSTBLOCK` ioctl command was to isolate the position from the geometry information, as from our (me and @Ouss4) interpretation it didn't make much sense to be together. But I had no stronger arguments to defend it.
> add a new IOCTL MTDIOC_PARTINFO?
It could be an option, but then much of the information to be retrieved via `MTDIOC_PARTINFO` would overlap with `MTDIOC_GEOMETRY`, being currently the only difference the `firstblock` info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
If the application already knows the path to the MTD, an `ioctl` call seems a more feasible approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679507386
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
The latter seems more reasonable, so that it would be possible to iterate the partition list, similar to what Linux has for MTD erase regions.
@xiaoxiang781216's suggestions for an `MTDIOC_PARTINFO` could also fit for when the caller already contains a direct reference to the partition (e.g. via a chardev path).
So, for an initial implementation, this could be the structure for a partition info:
```c
struct mtd_partition_info_s
{
size_t nblocks; /* Number of blocks in the partition */
off_t startblock; /* Offset to the first block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
For consistency, `part_ioctl` would then always forward the handling of `MTDIOC_GEOMETRY` to the parent MTD.
The application would now have the responsibility for calculating the partition size by invoking both `MTDIOC_GEOMETRY` (for `blocksize`) and `MTDIOC_PARTINFO` (for `nblocks`).
Is this approach reasonable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679507386
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
The latter seems more reasonable, so that it would be possible to iterate the partition list, similar to what Linux has for MTD erase regions.
@xiaoxiang781216's suggestions for an `MTDIOC_PARTINFO` could also fit for when the caller already contains a direct reference to the partition (e.g. via a chardev path).
So, for an initial implementation, this could be the structure for a partition info:
```c
struct mtd_partition_info_s
{
size_t nblocks; /* Number of blocks in the partition */
off_t startblock; /* Offset to the first block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
For consistency, `part_ioctl` would then always forward the handling of `MTDIOC_GEOMETRY` to the parent MTD.
The application would now have the responsibility for calculating the partition size by invoking both `MTDIOC_GEOMETRY` (for `blocksize`) and `MTDIOC_PARTINFO` (for `nblocks`).
Is this approach reasonable?
Edit: This change on `MTDIOC_GEOMETRY` would break applications. We may keep the current behavior for compatibility, but if agreed that GEOMETRY does not vary whether the MTD is a partition, we should schedule this change accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682900103
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> So 100% compatibility with Linux is not possible at the moment.
> But, the approach via `ioctl` is the most compatible to Linux.
That makes no sense to me. That is like saying I need a whale, but I don't have a whale. But a whale is mammal so I can use a bat instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682822415
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> That argument has value if the solution is 100% compatible with Linux.
Linux only provides the partition offset information via `sysfs`, a subsystem that NuttX does not currently have.
So 100% compatibility with Linux is not possible at the moment.
But, the approach via `ioctl` is the most compatible to Linux.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#issuecomment-895762783
@gustavonihei the block partition driver need update too:
https://github.com/apache/incubator-nuttx/blob/master/fs/driver/fs_blockpartition.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680116455
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
There has always been at least two methods for retrieving information from MTD:
- `ioctl` commands from the `MTDIOC_*`;
- `procfs`;
Currently neither approach provides the `firstblock` information.
So, consider an application that already owns the path to the MTD character device (via BCH and FTL) and already manipulates the MTD partition via file descriptors. So this is not one particular use case, it is a fairly common one.
Linux also provides this information via `ioctl` calls.
`procfs` requires `CONFIG_FS_PROCFS`, which is an optional feature. Adding `CONFIG_FS_PROCFS`, even after disabling all `procfs` entries except for the **partitions**, this imposes a footprint overhead of 2460 bytes to the binary file. There is also the runtime execution overhead of the two-way string conversion and lookup for the required information.
Do you really think it is worth paying the price for this coupling to `procfs`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679507386
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
The latter seems more reasonable, so that it would be possible to iterate the partition list, similar to what Linux has for MTD erase regions.
@xiaoxiang781216's suggestions for an `MTDIOC_PARTINFO` could also fit for when the caller already contains a direct reference to the partition (e.g. via a chardev path).
So, for an initial implementation, this could be the structure for a partition info:
```c
struct mtd_partition_info_s
{
size_t nblocks; /* Number of blocks in the partition */
off_t startblock; /* Offset to the first block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
For consistency, `part_ioctl` would then always forward the handling of `MTDIOC_GEOMETRY` to the parent MTD.
The application would now have the responsibility for calculating the partition size by invoking both `MTDIOC_GEOMETRY` (for `blocksize`) and `MTDIOC_PARTINFO` (for `nblocks`).
Is this approach reasonable?
**Edit**: This change on `MTDIOC_GEOMETRY` would break applications. We may keep the current behavior for compatibility, but if agreed that GEOMETRY should **not** vary whether the MTD is a partition, we should schedule this change accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680089235
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
One thing that I am not sure that I understand is how to associate a partition number with a block or MTD driver. In Linux, that might be done with the minor number of the device like /dev/sdaN where N is the partition number. But there are no real minor numbers in NuttX although such a convention could be adopted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
uint32_t magic; /* File system magic, 0 for RAW
* (see <sys/statfs.h>) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
size_t sectorsize; /* Size in bytes of a single sector */
off_t startsector; /* Offset to the first section/block of the
* managed sub-region */
off_t endsector; /* Offset to the last section/block of the
* managed sub-region */
FAR const char *parent; /* Name of the parent node of the partition */
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#issuecomment-890046231
> It's sad to hear that you believe I am bringing personal motivations
> to this PR. My original intent was to just provide one single piece of
> data to the application (the |firstblock| info pro MTD partition), and
> I am dedicating my time to work with you after each round of
> discussions, with the sole purpose of trying to do what's best to the
> project.
> So, please, I'd ask you to be a bit more reasonable.
>
Don't take it to seriously. I am just direct and just say what I
believe to be true. There is no malice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
If the application already knows the path to the MTD, an `ioctl` call seems a more feasible approach.
In my use case, the MTD is being managed via BCH and FTL layers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682923925
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs. I personally will not merge a non-standard, incompatible, redundant user solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r683121868
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
@gustavonihei it's reasonable to split a big change into the small patch, your propose is fine to me. Thanks for your effort to provide a general solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679937285
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
I've updated the PR with my interpretation from recent suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r685419432
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,12 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_PARTINFO _MTDIOC(0x000b) /* IN: Pointer to write-able struct
Review comment:
Done. I've also made FTL translate an incoming `BIOC_PARTINFO` into `MTDIOC_PARTINFO`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r685174116
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,12 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_PARTINFO _MTDIOC(0x000b) /* IN: Pointer to write-able struct
Review comment:
For me it does not seem fit to add it to the File-system IOCTL command group, since we're dealing with partitions here.
In my mind there were two possibilities:
1) Having both `MTDIOC_PARTINFO` and `BIOC_PARTINFO` which would behave similar to what we currently have for `MTDIOC_XIPBASE` and `BIOC_XIPBASE`.
2) Creating generic commands like Linux does, e.g. ` MEMGETPARTINFO`, which would be handled both by block devices and MTD.
I decided to stick to the first option because the second approach would require some refactoring on NuttX interfaces, which would cause some trouble to existing applications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r681970480
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Following the Linux path is the right way to go!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679245947
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Initially the idea of a new `MTDIOC_FIRSTBLOCK` ioctl command was to isolate the position from the geometry information, as from our (me and @Ouss4) interpretation it didn't make much sense to be together. But I had no stronger arguments to defend it.
I think you are correct. This is why a roadmap is critical. You cannot evaluate technical decisions today if you don't know where you want to be tomorrow.
"Tomorrow" we will want better partitiion support. We must plan for that today. Mixing volume and partition information is a horrible idea. We should not do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r681949575
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Some more evidences to justify the `ioctl` path:
1) Linux MTD character device driver implements similar `ioctl` commands (**MEMGETREGIONINFO and **MEMGETINFO**) for providing MTD info to the application:
https://github.com/torvalds/linux/blob/master/drivers/mtd/mtdchar.c#L684-L718
2) `mtd-utils` rely on **MEMGETINFO** for retrieving MTD partition information:
https://github.com/sigma-star/mtd-utils/blob/master/misc-utils/ftl_check.c#L80-L84
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682923925
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682806497
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> 1. Linux MTD character device driver implements similar `ioctl` commands (**MEMGETREGIONINFO** and **MEMGETINFO**) for providing MTD info to the application:
That argument has value if the solution is 100% compatible with Linux.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> So 100% compatibility with Linux is not possible at the moment.
> But, the approach via `ioctl` is the most compatible to Linux.
That makes no sense to me. That is like saying I need a whale, but I don't have a whale. But a whale is mammal so I can use a bat instead.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> So 100% compatibility with Linux is not possible at the moment.
> But, the approach via `ioctl` is the most compatible to Linux.
That makes no sense to me. That is like saying I need a whale, but I don't have a whale. But a whale is mammal so I can use a bat instead because it is also a mammal.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, redundant solution. But others may.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant solution. But others may.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant user solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs. I personally will not merge a non-standard, incompatible, redundant user solution. But others may.
I am going to get out of the loop on this one. So I will not be commenting further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682923925
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, incompatible, redundant solution. But others may.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#issuecomment-896042813
Ok, does anyone have more concern about this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680089452
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It seems like a VERY odd technical decision to duplicate an existing interface purely to satisfy your personal preference. Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
It's sad to hear that you believe I am bringing personal motivations to this PR. My original intent was to just provide one single piece of data to the application (the `firstblock` info from MTD partition), and I am dedicating my time to work with you after each round of discussions, with the sole purpose of trying to do what's best to the project.
So, please, I'd ask you to be a bit more reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r685169179
##########
File path: include/nuttx/fs/fs.h
##########
@@ -224,6 +224,20 @@ struct geometry
blksize_t geo_sectorsize; /* Size of one sector */
};
+struct partition_info_s
+{
+ uint32_t magic; /* File system magic, 0 for RAW
+ * (see <sys/statfs.h>) */
+ size_t length; /* Partition size in bytes */
+ size_t numsectors; /* Number of sectors in the partition */
+ size_t sectorsize; /* Size in bytes of a single sector */
+ off_t startsector; /* Offset to the first section/block of the
+ * managed sub-region */
+ off_t endsector; /* Offset to the last section/block of the
+ * managed sub-region */
+ FAR const char *parent; /* Name of the parent node of the partition */
Review comment:
Good catch. Done.
##########
File path: include/nuttx/fs/fs.h
##########
@@ -224,6 +224,20 @@ struct geometry
blksize_t geo_sectorsize; /* Size of one sector */
};
+struct partition_info_s
+{
+ uint32_t magic; /* File system magic, 0 for RAW
+ * (see <sys/statfs.h>) */
+ size_t length; /* Partition size in bytes */
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to split the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the `MTDIOC_GEOMETRY`, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#issuecomment-896006981
> @gustavonihei the block partition driver need update too:
> https://github.com/apache/incubator-nuttx/blob/master/fs/driver/fs_blockpartition.c
I thought we've already agreed on restricting the scope of this PR to just the MTD implementation to split the validation efforts.
Currently I have no testing setup for block partitions. Besides, there are no references to `register_blockpartition` on the whole repository.
The implementation seems rather trivial, but I'd like to avoid pushing it without proper testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires the a single information, don't you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679235473
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
add a new IOCTL MTDIOC_PARTINFO?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679604873
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
I should also mention..
For the case of MTD partitions, there is procfs support enabled by not excluding it with CONFIG_PROCFS_EXCLUDE_PARTITIONS. Ken Pettit contributed this ages ago but I know nearly nothing about this, but believe that procfs is the best way to read the partition table. It really shouldn't matter what kind of partition the device holds, as long as the procfs partition table format is the same.
This is why I say that if you know a partition table index, the you have complete knowledge of the partition... at least for the case of MTD.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679937285
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
I've updated the PR with my interpretation from recent suggestions. This could be extended for block devices and file systems as well via `BIOC_PARTINFO`/`FIOC_PARTINFO` commands to be handled by each driver.
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
I've updated the PR with my interpretation from recent suggestions. This could be extended for block devices and file systems as well via `BIOC_PARTINFO`/`FIOC_PARTINFO` commands to be handled by each implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682918726
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Please, explain which approach does make sense to you then.
I really hope that you have a better proposal than the string parsing approach via `procfs`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Add MTDIOC_FIRSTBLOCK for retrieving an MTD partition first block
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679142439
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
block device also need similar change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #4248: mtd: Add MTDIOC_FIRSTBLOCK for retrieving an MTD partition first block
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r678978735
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
Does it make sense to have it with `mtd_geometry_s`? Looks a bit out of the scope of what geometry means here. If we added it to mtd_geometry_s, wouldn't be 0 for most cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679507386
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
The latter seems more reasonable, so that it would be possible to iterate the partition list, similar to what Linux has for MTD erase regions.
@xiaoxiang781216's suggestions for an `MTDIOC_PARTINFO` could also fit for when the caller already contains a direct reference to the partition (e.g. via a chardev path).
So, for an initial implementation, this could be the structure for a partition info:
```c
struct mtd_partition_info_s
{
size_t neraseblocks; /* Number of blocks in the partition */
off_t startblock; /* Offset to the first block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
For consistency, `part_ioctl` would then always forward the handling of `MTDIOC_GEOMETRY` to the parent MTD.
The application would now have the responsibility for calculating the partition size by invoking both `MTDIOC_GEOMETRY` (for `blocksize`) and `MTDIOC_PARTINFO` (for `nblocks`).
Is this approach reasonable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679241798
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
Parititions all work differently. The Microsoft-ish partition tables used with SD cards and FAT aver very different from MTD partitions. Xaiomi added partition support of some kind too. It seems like a big mistake to put partition information into volume information. These are completely unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679527958
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
This think I replied to the wrong address. I will copy my response here if this is a duplicate and my email responses does show up, I will delete this:
> > It would make more sense to me, for example, if the geometry included a partition number perhaps. Then if there were APIs (or procfs/partition) to view the partitions, then all other information about the partition could be obtained from the more extensive partition table.
>
> Sorry, I couldn't follow your idea. By including "a partition number" you mean like an index? Or the number of partitions for the MTD?
Yes, I was thinking an index. I am not sure if that is really that meaningful.
>
> So, for an initial implementation, this could be the structure for a partition info:
> struct mtd_partition_info_s
> {
> size_t nblocks; /* Number of blocks in the partition */
> off_t startblock; /* Offset to the first block of the managed
> * sub-region */
> #ifdef CONFIG_MTD_PARTITION_NAMES
> FAR const char *name; /* Name of the partition */
> #endif
> };
Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
Bootable does not apply to NuttX, at least at present.
I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682806497
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> 1. Linux MTD character device driver implements similar `ioctl` commands (**MEMGETREGIONINFO** and **MEMGETINFO**) for providing MTD info to the application:
That argument has value if the solution is 100% compatible with Linux.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#issuecomment-895693729
LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
struct partition_info_s
{
uint32_t magic; /* File system magic, 0 for RAW
* (see <sys/statfs.h>) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
size_t sectorsize; /* Size in bytes of a single sector */
off_t startsector; /* Offset to the first section/block of the
* managed sub-region */
off_t endsector; /* Offset to the last section/block of the
* managed sub-region */
FAR const char *parent; /* Name of the parent node of the partition */
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Add MTDIOC_FIRSTBLOCK for retrieving an MTD partition first block
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679041908
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
The mtd device is always zero, but partition on mtd isn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4248: mtd: Add MTDIOC_FIRSTBLOCK for retrieving an MTD partition first block
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r678800451
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
how about block device?
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
How about add the new field to mtd_geometry_s or geometry? the start of block is also a key metric for block/mtd geometry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680116455
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
There has always been at least two methods for retrieving information from MTD:
- `ioctl` commands from the `MTDIOC_*`;
- `procfs`;
Currently neither approach provides the `firstblock` information.
So, consider an application that already owns the path to the MTD character device and already manipulates the MTD partition via file descriptors (via BCH and FTL). So this is not one particular use case, it is a fairly common one.
Linux also provides this information via `ioctl` calls.
`procfs` requires `CONFIG_FS_PROCFS`, which is an optional feature. Adding `CONFIG_FS_PROCFS`, even after disabling all `procfs` entries except for the **partitions**, this imposes a footprint overhead of 2460 bytes to the binary file. There is also the runtime execution overhead of the two-way string conversion and lookup for the required information.
Do you really think it is worth paying the price for this coupling to `procfs`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682923925
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I really hope that you have a better proposal than the string parsing approach via `procfs`.
I would prefer in this order: A 100% Linux compatible solution, string parsing via procfs. I personally will not merge a non-standard, redundant solution. But others may.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to divide the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the MTDIOC_GEOMETRY, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
uint32_t magic; /* File-system magic (0 for RAW) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
off_t startsector; /* Offset to the first block/section of the managed
* sub-region */
off_t endsector; /* Offset to the last block of the managed
* sub-region */
FAR const char *parent; /* Parent node of the partition */
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682822415
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> That argument has value if the solution is 100% compatible with Linux.
Linux only provides the partition offset information via `sysfs`, a subsystem that NuttX does not currently have.
https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-mtd#L226-L234
So 100% compatibility with Linux is not possible at the moment.
But, the approach via `ioctl` is the most compatible to Linux.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682918726
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Please, explain which approach does make sense to you then.
I really hope that you have a better proposal than the string parsing approach via `procfs`.
Remember the original intent of this PR, which is to provide the first sector information from an MTD partition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679219317
##########
File path: include/nuttx/mtd/mtd.h
##########
@@ -74,6 +74,9 @@
#define MTDIOC_ERASESTATE _MTDIOC(0x000a) /* IN: Pointer to uint8_t
* OUT: Byte value that represents the
* erased state of the MTD cell */
+#define MTDIOC_FIRSTBLOCK _MTDIOC(0x000b) /* IN: Pointer to off_t
Review comment:
> block device also need similar change.
I've just seen that block device partitions also handle `MTDIOC_GEOMETRY`, is this what you were referring to?
https://github.com/apache/incubator-nuttx/blob/fd19c36f5b7d36fb5b19946e035b5bff2d1b8628/fs/driver/fs_blockpartition.c#L249
Or do you mean to also add a new field to `struct geometry`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682900103
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> So 100% compatibility with Linux is not possible at the moment.
> But, the approach via `ioctl` is the most compatible to Linux.
That makes no sense to me. That is like saying I need a whale, but I don't have a whale. But a whale is mammal so I can use a bat instead because it is also a mammal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679946084
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
This is redundant. This information is all available in the procfs/. See https://github.com/apache/incubator-nuttx/blob/master/drivers/mtd/mtd_partition.c#L617
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct partition_info_s
{
uint32_t magic; /* File system magic, 0 for RAW
* (see <sys/statfs.h>) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
off_t startsector; /* Offset to the first block/section of the managed
* sub-region */
off_t endsector; /* Offset to the last block of the managed
* sub-region */
FAR const char *parent; /* Name of the parent node of the partition */
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680084472
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
It seems like a VERY odd technical decision to duplicate an existing interface purely to satisfy your personal preference. Redundancy like that should not be tolerated in an OS and has never been tolerated in the past.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`?
Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
If the application already knows the path to the MTD, an `ioctl` call seems a more feasible approach.
In my use case, the MTD is being managed via BCH and FTL layers.
I believe it is reasonable to provide both approaches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679227094
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
Is firstblock even meaningful with some partition types. I think mixing partitions and volume geometry is not a good idea.
Rather, it would make more sense to provide more complete partition information as you get from a partition tool like fdisk. Again, we need to think of a roadmap. Where do you want to go with this? I don't think this gets your there. It mixes functionality and cannot be built upon for more general partition support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682981692
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs.
A 100% Linux compatible solution implies the creation of a `sysfs` subsystem for NuttX. Although much desirable, unfortunately is far beyond the scope for this PR.
And string parsing via `procfs` when compared to `ioctl` solution is sub-optimal in terms of execution time and code size. Execution time will scale depending on the number of partitions to be returned from `/proc/partitions`. Code size will increase due to the requirement of enabling the optional `procfs` subsystem.
Besides the fact that the only available method for identifying a partition from `/proc/partitions` is via the partition name, which is once again sub-optimal considering it involves comparison of strings.
Using `ioctl` only requires a valid file descriptor and does not impose any architectural changes to the OS, since the MTD subsystem already implements several other `ioctl` commands. Also, the value to be returned via the `ioctl` command is readily available in the MTD partition information.
@xiaoxiang781216 Please kindly evaluate the answers from https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813 and also the above reasoning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682981692
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs.
A 100% Linux compatible solution implies the creation of a `sysfs` subsystem for Linux. Although much desirable, unfortunately is far beyond the scope for this PR.
And string parsing via `procfs` when compared to `ioctl` solution is sub-optimal in terms of execution time and code size. Execution time will scale depending on the number of partitions to be returned from `/proc/partitions`. Code size will increase due to the requirement of enabling the optional `procfs` subsystem.
Besides the fact that the only available method for identifying a partition from `/proc/partitions` is via the partition name, which is once again sub-optimal considering it involves comparison of strings.
Using `ioctl` only requires a valid file descriptor and does not impose any architectural changes to the OS, since the MTD subsystem already implements several other `ioctl` commands. Also, the value to be returned via the `ioctl` command is readily available in the MTD partition information.
@xiaoxiang781216 Please kindly evaluate the answers from https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813 and also the above reasoning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679892023
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
> Think of what you see if you list partitions with fdisk or other partition tool. You would want, for example, the filesystem type if there is a file system in the partition. This comes from the magic number at the beginning of the partition (as in include/sys/statfs.h).
>
> With fdisk -l you would see the device, an indication of the volume is bootable, start sector, end sector, size in setions, size in human-readable bytes, ID (https://www.win.tue.nl/~aeb/partitions/partition_types-1.html), and human readable type.
>
> MTD makes this more complex, however, since MTD devices to do not have partition tables; mtd_partition.c is a software layer that imposes the partition layout on the device with nothing stored on the media. The good part of that design is that it is very light weight; the bad part is that only the firmware knows the partition layout of the FLASH memory. There is no way to move an MTD part from on platform to another while retaining the partition information. That should probably be made more standard someday.
Ok, so for an MTD partition the proposed struct seems fit, maybe just an `endblock` field and change the `nblocks` to a `length` in bytes.
> I believe that reading the partition table from a block device is a user application. Nothing special is required from the OS since the partition table can be directly read from the block device by a user-space application.
Agreed. The information from a single partition (and the partition table, as well) can be retrieved both via IOCTL calls or via `procfs`. For the use case I am dealing with, an `ioctl` call seems more suitable. I believe the `procfs` approach fits best from NSH usage.
So your idea is that we should have a generic `struct partition_info`, not specific to MTD. Basically that would be the previous `mtd_partition_info` with added info, something like:
```c
struct mtd_partition_info_s
{
uint32_t magic; /* File-system magic (0 for RAW) */
size_t length; /* Partition size in bytes */
size_t numsectors; /* Number of sectors in the partition */
off_t startsector; /* Offset to the first block/section of the managed
* sub-region */
off_t endsector; /* Offset to the last block of the managed
* sub-region */
#ifdef CONFIG_MTD_PARTITION_NAMES
FAR const char *name; /* Name of the partition */
#endif
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r680061475
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
We had that discussion years ago. When Ken created procfs, some people want it to be human readable strings (like Linux) and others wanted a binary interface. It was resolved then that we would use the string interface and that we would pay that price. This is a resolved issue and there is really no point in raising it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to divide the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the MTDIOC_GEOMETRY, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to divide the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the `MTDIOC_GEOMETRY`, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> It's good to follow Linux design, two question:
>
> 1. Should we also use MEMGETINFO and mtd_info_t too?
For compatibility purposes, I think we should.
On Linux `mtd_info_t` is a typedef to `struct mtd_info_user` and some of its fields overlap with NuttX `mtd_geometry_s`. So this would require to refactor `MTDIOC_GEOMETRY` to avoid having two `ioctl` commands returning the same information.
Since this is not a simple task, I'd suggest to refactor it in another PR.
> 2. How about block driver which should have the similar requirement too?
I also agree on this one.
The intent with the proposed `struct partition_info_s` was to make it generic, so that it could be used by block devices also.
But, again, I'd like to restrict the scope of this PR to MTD only to split the validation effort.
I believe that the proposed `MTDIOC_PARTINFO` does not overlap with `MEMGETINFO` nor `MEMGETREGIONINFO`. As explained by @patacongo, the MTD partition abstraction is more of a software abstraction than a real partition, and it is different to what Linux refers to a MTD Region, which means areas of an MTD with distinct erase sizes, something that we don't have in NuttX so far (to my knowledge).
So merging this proposal would not be a wasted effort. We may later have both `MEMGETINFO` for compatibility purposes by refactoring the `MTDIOC_GEOMETRY`, and also `MEMGETREGIONINFO` if eventually we support these types of MTD with more than one erase region.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> That argument has value if the solution is 100% compatible with Linux.
Linux only provides the partition offset information via `sysfs`, a subsystem that NuttX does not currently have.
So 100% compatibility with Linux is not possible at the moment.
But, the approach via `ioctl` is the most compatible to Linux.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> That argument has value if the solution is 100% compatible with Linux.
Linux only provides the partition offset information via `sysfs`, a subsystem that NuttX does not currently have.
https://github.com/torvalds/linux/blob/master/Documentation/ABI/testing/sysfs-class-mtd#L226-L234
So 100% compatibility with Linux is not possible at the moment.
But, the approach via `ioctl` is the most compatible to Linux.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Please, explain which approach does make sense to you then.
I really hope that you have a better proposal than the string parsing approach via `procfs`.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Please, explain which approach does make sense to you then.
I really hope that you have a better proposal than the string parsing approach via `procfs`.
Remember the original intent of this PR, which is to provide the first sector information from an MTD partition.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs.
A 100% Linux compatible solution implies the creation of a `sysfs` subsystem for Linux. Although much desirable, unfortunately is far beyond the scope for this PR.
And string parsing via `procfs` when compared to `ioctl` solution is sub-optimal in terms of execution time and code size. Execution time will scale depending on the number of partitions to be returned from `/proc/partitions`. Code size will increase due to the requirement of enabling the optional `procfs` subsystem.
Besides the fact that the only available method for identifying a partition from `/proc/partitions` is via the partition name, which is once again sub-optimal considering it involves comparison of strings.
Using `ioctl` only requires a valid file descriptor and does not impose any architectural changes to the OS, since the MTD subsystem already implements several other `ioctl` commands. Also, the value to be returned via the `ioctl` command is readily available in the MTD partition information.
@xiaoxiang781216 Please kindly evaluate the answers from https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813 and also the above reasoning.
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
> I would prefer in this order: A 100% Linux compatible solution then string parsing via the existing procfs.
A 100% Linux compatible solution implies the creation of a `sysfs` subsystem for NuttX. Although much desirable, unfortunately is far beyond the scope for this PR.
And string parsing via `procfs` when compared to `ioctl` solution is sub-optimal in terms of execution time and code size. Execution time will scale depending on the number of partitions to be returned from `/proc/partitions`. Code size will increase due to the requirement of enabling the optional `procfs` subsystem.
Besides the fact that the only available method for identifying a partition from `/proc/partitions` is via the partition name, which is once again sub-optimal considering it involves comparison of strings.
Using `ioctl` only requires a valid file descriptor and does not impose any architectural changes to the OS, since the MTD subsystem already implements several other `ioctl` commands. Also, the value to be returned via the `ioctl` command is readily available in the MTD partition information.
@xiaoxiang781216 Please kindly evaluate the answers from https://github.com/apache/incubator-nuttx/pull/4248#discussion_r682578813 and also the above reasoning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4248: mtd: Return MTD first block info on MTDIOC_GEOMETRY
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679525929
##########
File path: arch/arm/src/cxd56xx/cxd56_sfc.c
##########
@@ -200,6 +200,7 @@ static int cxd56_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
geo->blocksize = PAGE_SIZE;
geo->erasesize = SECTOR_SIZE;
geo->neraseblocks = priv->density >> SECTOR_SHIFT;
+ geo->firstblock = 0;
Review comment:
Another, unrelated partition design problem is the way that the Microsoft DOS partitions are managed (e.g. on SD cards). There is no standalone partition logic; the partition logic is built into the FAT file system. If the first block is not the start of the FAT file system, then it scans the partition table and mounts the first FAT file system if finds.
That raises some problems. It means, for example, that we cannot handle two FAT file systems in different partitions and that it would be be awkward to use the other partitions for anything else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4248: mtd: Return MTD Partition Information
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4248:
URL: https://github.com/apache/incubator-nuttx/pull/4248#discussion_r679951763
##########
File path: drivers/mtd/mtd_partition.c
##########
@@ -413,6 +413,25 @@ static int part_ioctl(FAR struct mtd_dev_s *dev, int cmd, unsigned long arg)
}
break;
+ case MTDIOC_PARTINFO:
+ {
+ FAR struct partition_info_s *info =
+ (FAR struct partition_info_s *)arg;
+ if (info != NULL)
+ {
+ info->magic = 0;
+ info->numsectors = priv->neraseblocks * priv->blkpererase;
+ info->length = info->numsectors * priv->blocksize;
+ info->sectorsize = priv->blocksize;
+ info->startsector = priv->firstblock;
+ info->endsector = priv->firstblock + info->numsectors;
+ info->parent = priv->parent->name;
+
+ ret = OK;
+ }
+ }
+ break;
+
Review comment:
Yes, but how should the application consume this information from `procfs`? Please, correct if I misunderstood it, but a read access via `procfs` retrieves the information as string. So the application should parse this info. If so, this is sub-optimal for an application that just requires a single information, don't you think?
If the application already knows the path to the MTD, an `ioctl` call seems a more feasible approach.
In my use case, the MTD is being managed via BCH and FTL layers.
I believe it is reasonable to provide both approaches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org