On 2/24/2023 8:25 AM, Stanislaw Gruszka wrote:
On Mon, Feb 06, 2023 at 08:41:42AM -0700, Jeffrey Hugo wrote:
+#define SEM_VAL_MASK   GENMASK_ULL(11, 0)
+#define SEM_INDEX_MASK GENMASK_ULL(4, 0)
+#define BULK_XFER      BIT(3)
+#define GEN_COMPLETION BIT(4)
+#define INBOUND_XFER   1
+#define OUTBOUND_XFER  2
+#define REQHP_OFF      0x0 /* we read this */
+#define REQTP_OFF      0x4 /* we write this */
+#define RSPHP_OFF      0x8 /* we write this */
+#define RSPTP_OFF      0xc /* we read this */
+
+#define ENCODE_SEM(val, index, sync, cmd, flags)                       \
+                       ((val) |                                        \
+                       (index) << 16 |                                   \
+                       (sync) << 22 |                                    \
+                       (cmd) << 24 |                                     \
+                       ((cmd) ? BIT(31) : 0) |                         \
+                       (((flags) & SEM_INSYNCFENCE) ? BIT(30) : 0) |       \
+                       (((flags) & SEM_OUTSYNCFENCE) ? BIT(29) : 0))

This could be probably better coded using FIELD_PREP()
with integrated checks of passed values not exceed
field width.

Interesting idea.  Will have a look.


+struct dbc_req { /* everything must be little endian encoded */

This comment does not provide much value IMHO ...

With the LE types, this does seem to be redundant now.  Will remove.


+inline int get_dbc_req_elem_size(void)
+{
+       return sizeof(struct dbc_req);
+}
+
+inline int get_dbc_rsp_elem_size(void)
+{
+       return sizeof(struct dbc_rsp);
+}

.. as those those inliners, instead of using sizeof() directly.
Up to you to change.

Will review these.


+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().

+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;
+                       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. Are you suggesting renaming this function? I guess I'm not quite understanding your comment here. Can you elaborate?

Reply via email to