On Wed, Oct 20, 2021 at 04:29:39PM +0800, Chen Yu wrote:
> > > +ssize_t pfru_log_read(struct file *filp, char __user *ubuf,
> > > + size_t size, loff_t *off)
> > > +{
> > > + struct pfru_log_data_info info;
> > > + phys_addr_t base_addr;
> > > + int buf_size, ret;
> > > + char *buf_ptr;
> > > +
> > > + if (!pfru_log_dev)
> > > + return -ENODEV;
> > > +
> > > + if (*off < 0)
> > > + return -EINVAL;
> > > +
> > > + ret = get_pfru_log_data_info(&info, pfru_log_dev->info.log_type);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + base_addr = (phys_addr_t)(info.chunk2_addr_lo | (info.chunk2_addr_hi << 32));
> > > + /* pfru update has not been launched yet.*/
> > > + if (!base_addr)
> > > + return -EBUSY;
> > > +
> > > + buf_size = info.max_data_size;
> > > + if (*off >= buf_size)
> > > + return 0;
> > > +
> > > + buf_ptr = memremap(base_addr, buf_size, MEMREMAP_WB);
> > > + if (IS_ERR(buf_ptr))
> > > + return PTR_ERR(buf_ptr);
> > > +
> > > + size = min_t(size_t, size, buf_size - *off);
> > > + if (copy_to_user(ubuf, buf_ptr + *off, size))
> > > + ret = -EFAULT;
> > > + else
> > > + ret = 0;
> >
> > As all you are doing is mapping some memory and reading from it, why do
> > you need a read() file operation at all? Why not just use mmap?
> >
> In the beginning mmap() interface was provided to the user. Then it was
> realized that there is no guarantee in the spec that, the physical address
> provided by the BIOS would remain unchanged. So instead of asking the user
> to mmap the file each time before reading the log, the read() is leveraged
> here to always memremap() the latest address.
So you are forced to memremap on _EVERY_ read call because the BIOS can
change things underneath us without the kernel knowing about it? How
does the chunk2_addr_lo and _hi values change while the system is
running? Where does that happen, and isn't this going to be a very slow
and expensive read call all the time?
thanks,
greg k-h
