On Tue, Oct 20, 2020 at 02:30:07PM +0800, Zou Cao wrote:
> +ssize_t regfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> +{
> + struct file *file = iocb->ki_filp;
> + struct inode *inode = file->f_mapping->host;
> + ssize_t ret;
> +
> + inode_lock(inode);
> + ret = generic_write_checks(iocb, from);
> + if (ret > 0)
> + ret = __generic_file_write_iter(iocb, from);
> + inode_unlock(inode);
> +
> + if (ret > 0)
> + ret = generic_write_sync(iocb, ret);
> + return ret;
> +}
Huh? How is that different from generic_file_write_iter()? And who's
using it, anyway?
> + struct regfs_inode_info *info = REGFS_I(mapping->host);
> + char str[67];
> + unsigned long val = 0;
> + loff_t pos = *ppos;
> + size_t res;
> +
> + if (pos < 0)
> + return -EINVAL;
> + if (pos >= len || len > 66)
> + return 0;
This is completely bogus. "If current position is greater than the
length of string we are asking to write, quietly return 0"?
> + res = copy_from_user(str, buf, len);
> + if (res)
> + return -EFAULT;
> + str[len] = 0;
> +
> + if (kstrtoul(str, 16, &val) < 0)
> + return -EINVAL;
Where does 67 come from? If you are expecting a hexadecimal representation
of a unsigned long on arm64, you should have at most 16 digits. 67 looks
rather odd...
> + writel_relaxed(val, info->base + info->offset);
... and you are promptly discarding the upper 32 bits, since writel_relaxed()
takes u32:
((void)__raw_writel((__force u32)cpu_to_le32(v),(c)))
is going to truncate to 32bit, no matter what. Quietly truncate, at that...
> +const struct address_space_operations regfs_aops = {
> + .readpage = simple_readpage,
> + .write_begin = simple_write_begin,
> + .write_end = simple_write_end,
> + .set_page_dirty = __set_page_dirty_buffers,
> +};
Again, huh? What would use the page cache there, anyway?
> +static LIST_HEAD(regfs_head);
Protected by...?
> +static const struct inode_operations regfs_dir_inode_operations;
> +int regfs_debug;
> +module_param(regfs_debug, int, S_IRUGO);
> +MODULE_PARM_DESC(regfs_debug, "enable regfs debug mode");
> +
> +struct inode *regfs_get_inode(struct super_block *sb, const struct inode
> *dir, umode_t mode, dev_t dev)
> +{
> + struct inode *inode = new_inode(sb);
> +
> + if (inode) {
> + inode->i_ino = get_next_ino();
> + inode_init_owner(inode, dir, mode);
> + inode->i_mapping->a_ops = ®fs_aops;
> + //inode->i_mapping->backing_dev_info = ®fs_backing_dev_info;
> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> + mapping_set_unevictable(inode->i_mapping);
> + inode->i_atime = inode->i_mtime = inode->i_ctime =
> current_time(inode);
> + switch (mode & S_IFMT) {
> + default:
> + init_special_inode(inode, mode, dev);
> + break;
> + case S_IFREG:
> + inode->i_op = ®fs_file_inode_operations;
> + inode->i_fop = ®fs_file_operations;
> + break;
> + case S_IFDIR:
> + inode->i_op = ®fs_dir_inode_operations;
> + inode->i_fop = &simple_dir_operations;
> +
> + /* directory inodes start off with i_nlink == 2 (for
> "." entry) */
> + inc_nlink(inode);
> + break;
> + case S_IFLNK:
> + inode->i_op = &page_symlink_inode_operations;
> + break;
> + }
> + }
> +
> + return inode;
> +}
Seriously? Where would symlinks, device nodes, FIFOs and sockets come from?
And you are open-coding the regular file case in the new_dentry_create() anyway,
so the only thing this is actually used for is the root directory.
> +static const struct inode_operations regfs_dir_inode_operations = {
> + .lookup = simple_lookup,
> +};
... and simple_dir_inode_operations is wrong, because...?
> +static struct dentry *new_dentry_create(struct super_block *sb, struct
> dentry *parent,
> + const char *name, bool is_dir, struct res_data *res)
> +{
> + struct dentry *dentry;
> + struct inode *inode;
> + struct regfs_inode_info *ei;
> + struct regfs_fs_info *fsi = sb->s_fs_info;
> +
> + dentry = d_alloc_name(parent, name);
> + if (!dentry)
> + return NULL;
> +
> + inode = new_inode(sb);
> + if (!inode)
> + goto out;
> +
> + ei = REGFS_I(inode);
> + inode->i_ino = get_next_ino();;
> + inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> + inode->i_uid = GLOBAL_ROOT_UID;
> + inode->i_gid = GLOBAL_ROOT_GID;
> + if (is_dir) {
> + inode->i_mode = S_IFDIR | S_IRUGO | S_IWUSR;
> + inode->i_op = ®fs_dir_inode_operations;
> + inode->i_fop = &simple_dir_operations;
> + list_add(&ei->list, &fsi->list);
where's the matching removal from the list?
> + } else {
> + inode->i_mode = S_IFREG | S_IRUGO | S_IWUSR;
> + inode->i_op = ®fs_file_inode_operations;
> + inode->i_fop = ®fs_file_operations;
> + inc_nlink(inode);
> + }
> + ei->base = (void *)res->base;
> + ei->offset = res->offset;
> + ei->type = res->type;
> +
> + d_add(dentry, inode);
> +
> + loc_debug("new dentry io base:%llx offset:%llx ei:%llx\n",
> (u64)ei->base, (u64)ei->offset, (u64)ei);
> + return dentry;
> +out:
> + dput(dentry);
> + return NULL;
> +}
> +
> +static void node_transfer_dentry(struct super_block *sb)
> +{
> + struct regfs_fs_info *fsi = sb->s_fs_info;
> + void *blob = fsi->dtb_buf;
> + const char *pathp;
> + int node_offset, depth = -1;
> + struct dentry *parent = NULL;
> + u64 parent_base;
> +
> + for (node_offset = fdt_next_node(blob, -1, &depth);
> + node_offset >= 0 && depth >= 0;
> + node_offset = fdt_next_node(blob, node_offset, &depth)) {
> +
> + const struct fdt_property *prop;
> + struct res_data res;
> +
> + pathp = fdt_get_name(blob, node_offset, NULL);
> + prop = (void *)fdt_getprop(blob, node_offset, "reg", NULL);
> +
> + if (prop) {
> + unsigned long phys;
> +
> + phys = fdt32_to_cpu(((const __be32 *)prop)[1]);
> + res.type = RES_TYPE_RANGE;
> + res.offset = fdt32_to_cpu(((const __be32 *)prop)[3]);
> + res.base = (u64)ioremap(phys, res.offset);
> +
> + if (!res.base) {
> + parent = NULL;
> + parent_base = 0;
> + continue;
> + }
> +
> + loc_debug("%s reg:%lx size:%lx map:%llx\n\n", pathp
> + , (unsigned long) fdt32_to_cpu(((const __be32
> *)prop)[1])
> + , (unsigned long) fdt32_to_cpu(((const __be32
> *)prop)[3])
> + , (u64)res.base);
> +
> + parent = new_dentry_create(sb, sb->s_root, (const char
> *)pathp, true, &res);
> + parent_base = res.base;
> +
> + } else {
> + // parent dentry is create failed, igonre all child
> dentry
> + if (!parent)
> + continue;
> +
> + prop = (void *)fdt_getprop(blob, node_offset, "offset",
> NULL);
> + if (prop) {
> +
> + res.offset = fdt32_to_cpu(*(const __be32
> *)prop);
> + res.base = parent_base;
> + res.type = RES_TYPE_ITEM;
> +
> + new_dentry_create(sb, parent, (const char *)
> pathp, false, &res);
> + loc_debug("%s offset:%lx\n", pathp, (unsigned
> long)fdt32_to_cpu(*(const __be32 *)prop));
> + }
> + }
> + }
> +}
> +
> +static int parse_options(char *options, struct super_block *sb)
> +{
> + char *p;
> + int ret = -EINVAL;
> + struct regfs_fs_info *fsi;
> + size_t msize = INT_MAX;
> +
> + fsi = sb->s_fs_info;
> +
> + if (!options)
> + return -EINVAL;
> +
> + while ((p = strsep(&options, ",")) != NULL) {
> + char *name, *name_val;
> +
> + name = strsep(&p, "=");
> + if (name == NULL)
> + goto failed;
> +
> + name_val = strsep(&p, "=");
> + if (name_val == NULL)
> + goto failed;
> +
> + //get resource address
> + if (!strcmp(name, "dtb")) {
> + ret = kernel_read_file_from_path(name_val,
> &fsi->dtb_buf, &fsi->dtb_len, msize, READING_UNKNOWN);
Why bother doing that in the kernel?
> +struct dentry *regfs_mount(struct file_system_type *fs_type,
> + int flags, const char *dev_name, void *data)
> +{
> + struct dentry *root_dentry;
> + struct super_block *sb;
> +
> + root_dentry = mount_nodev(fs_type, flags, data, regfs_fill_super);
> +
> + sb = root_dentry->d_sb;
> +
> + if (sb->s_root) {
> + node_transfer_dentry(sb);
Er... Why not do that in regfs_fill_super()?
Al, not going any further for now...
