On 2/27/2023 10:14 AM, Stanislaw Gruszka wrote:
On Fri, Feb 24, 2023 at 12:36:51PM -0700, Jeffrey Hugo wrote:
+static int reserve_pages(unsigned long start_pfn, unsigned long nr_pages,
+ bool reserve)
+{
+ unsigned long pfn;
+ unsigned long end_pfn = start_pfn + nr_pages;
+ struct page *page;
+
+ for (pfn = start_pfn; pfn < end_pfn; pfn++) {
+ if (!pfn_valid(pfn))
+ return -EINVAL;
+ page = pfn_to_page(pfn);
+ if (reserve)
+ SetPageReserved(page);
+ else
+ ClearPageReserved(page);
It is needed? Looks like taken from some legacy code.
Required for remap_pfn_range().
PG_reserved is not required any longer for remap_pfn_range(), here
is excerpt from comment from include/linux/page-flags.h :
* Some PG_reserved pages will be excluded from the hibernation image.
* PG_reserved does in general not hinder anybody from dumping or swapping
* and is no longer required for remap_pfn_range(). ioremap might require it.
* Consequently, PG_reserved for a page mapped into user space can indicate
* the zero page, the vDSO, MMIO pages or device memory.
I clearly missed that and was relying on other documentation. Thank you
for pointing this out. Will remove.
+static int copy_sgt(struct qaic_device *qdev, struct sg_table **sgt_out,
+ struct sg_table *sgt_in, u64 size, u64 offset)
+{
+ int total_len, len, nents, offf = 0, offl = 0;
+ struct scatterlist *sg, *sgn, *sgf, *sgl;
+ struct sg_table *sgt;
+ int ret, j;
+
+ /* find out number of relevant nents needed for this mem */
+ total_len = 0;
+ sgf = NULL;
+ sgl = NULL;
+ nents = 0;
+
+ size = size ? size : PAGE_SIZE;
+ for (sg = sgt_in->sgl; sg; sg = sg_next(sg)) {
+ len = sg_dma_len(sg);
+
+ if (!len)
+ continue;
+ if (offset >= total_len && offset < total_len + len) {
+ sgf = sg;
+ offf = offset - total_len;
+ }
+ if (sgf)
+ nents++;
+ if (offset + size >= total_len &&
+ offset + size <= total_len + len) {
+ sgl = sg;
+ offl = offset + size - total_len;
+ break;
+ }
+ total_len += len;
+ }
+
+ if (!sgf || !sgl) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
+ if (!sgt) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
+ if (ret)
+ goto free_sgt;
+
+ /* copy relevant sg node and fix page and length */
+ sgn = sgf;
+ for_each_sgtable_sg(sgt, sg, j) {
+ memcpy(sg, sgn, sizeof(*sg));
+ if (sgn == sgf) {
+ sg_dma_address(sg) += offf;
This looks a bit suspicious. Are you sure you can modify
sg->dma_address and still use it as valid value ?
A single entry in the sg table is a contiguous mapping of memory. If it
wasn't contiguous, it would have to be broken up into multiple entries.
In the simple case, a driver is going to take the dma_address/len pair
and hand that directly to the device. Then the device is going to
access every address in that range.
If the device can access every address from dma_address to dma_address +
len, why can't it access a subset of that?
+ sg_dma_len(sg) -= offf;
+ sg_set_page(sg, sg_page(sgn),
+ sg_dma_len(sg), offf);
+ } else {
+ offf = 0;
+ }
+ if (sgn == sgl) {
+ sg_dma_len(sg) = offl - offf;
+ sg_set_page(sg, sg_page(sgn),
+ offl - offf, offf);
+ sg_mark_end(sg);
+ break;
+ }
+ sgn = sg_next(sgn);
+ }
Why not use sg_copy_table() ? Overall copy_sgt() seems to be something
that could be replaced by generic helper or at least simplify.
I don't see "sg_copy_table" defined in 6.2.
Because there is no such function in any kernel source. It was only my
imagination, not sure right now how I came up with this function name :-/
Sorry about confusion.
There are only sg_copy_{to,from}_buffer(), but not really useful in
this case.
Are you suggesting renaming
this function? I guess I'm not quite understanding your comment here. Can
you elaborate?
Renaming would be nice. I was thinking by simplifying it, not sure
now if that's easy achievable, though.
Ok. I'll think on this.
