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:

quoted

quoted

quoted

+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