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.
> +struct dbc_req { /* everything must be little endian encoded */
This comment does not provide much value IMHO ...
> +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.
> +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.
> +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.
Regards
Stanislaw
