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.
> > > +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 ?
> > > + 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.
Regards
Stanislaw
