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

[Chen Yu <yu.c.chen@intel.com>]

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?
>
It was not documented in the spec whether the chunk address will change or not,
for safety I changed it from mmap() to read(). I'll try to reach the spec designer
and check if the address will change or not.

thanks,
Chenyu
 
> thanks,
> 
> greg k-h