Re: [PATCH v4 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

[Mike Rapoport]

On Wed, Oct 20, 2021 at 10:27:28AM +0200, Greg Kroah-Hartman wrote:
> 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?

And maybe it is something that can be fixed in the spec so that the address
will remain constant?

(I realise it's wishful thinking, but sill...)

-- 
Sincerely yours,
Mike.