On Mon, Jul 19, 2021 at 08:31:05PM -0700, longli@linuxonhyperv.com wrote:

+struct az_blob_device {
+	struct hv_device *device;
+
+	/* Opened files maintained by this device */
+	struct list_head file_list;
+	/* Lock for protecting file_list */
+	spinlock_t file_lock;
+
+	/* The refcount for this device */
+	refcount_t count;

Just use a kref please if you really need this.  Are you sure you do?
You already have 2 other reference counted objects being used here, why
make it 3?

+	/* Pending requests to VSP */
+	atomic_t pending;

Why does this need to be atomic?

+	wait_queue_head_t waiting_to_drain;
+
+	bool removing;

Are you sure this actually works properly?  Why is it needed vs. any
other misc device?

+/* VSC->VSP request */
+struct az_blob_vsp_request {
+	u32 version;
+	u32 timeout_ms;
+	u32 data_buffer_offset;
+	u32 data_buffer_length;
+	u32 data_buffer_valid;
+	u32 operation_type;
+	u32 request_buffer_offset;
+	u32 request_buffer_length;
+	u32 response_buffer_offset;
+	u32 response_buffer_length;
+	guid_t transaction_id;
+} __packed;

Why packed?  If this is going across the wire somewhere, you need to
specify the endian-ness of these values, right?  If this is not going
across the wire, no need for it to be packed.

+
+/* VSP->VSC response */
+struct az_blob_vsp_response {
+	u32 length;
+	u32 error;
+	u32 response_len;
+} __packed;

Same here.

+
+struct az_blob_vsp_request_ctx {
+	struct list_head list;
+	struct completion wait_vsp;
+	struct az_blob_request_sync *request;
+};
+
+struct az_blob_file_ctx {
+	struct list_head list;
+
+	/* List of pending requests to VSP */
+	struct list_head vsp_pending_requests;
+	/* Lock for protecting vsp_pending_requests */
+	spinlock_t vsp_pending_lock;
+	wait_queue_head_t wait_vsp_pending;
+
+	pid_t pid;

Why do you need a pid?  What namespace is this pid in?

+static int az_blob_probe(struct hv_device *device,
+			 const struct hv_vmbus_device_id *dev_id)
+{
+	int ret;
+	struct az_blob_device *dev;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	spin_lock_init(&dev->file_lock);
+	INIT_LIST_HEAD(&dev->file_list);
+	atomic_set(&dev->pending, 0);
+	init_waitqueue_head(&dev->waiting_to_drain);
+
+	ret = az_blob_connect_to_vsp(device, dev, AZ_BLOB_RING_SIZE);
+	if (ret)
+		goto fail;
+
+	refcount_set(&dev->count, 1);
+	az_blob_dev = dev;
+
+	// create user-mode client library facing device
+	ret = az_blob_create_device(dev);
+	if (ret) {
+		dev_err(AZ_DEV, "failed to create device ret=%d\n", ret);
+		az_blob_remove_vmbus(device);
+		goto fail;
+	}
+
+	dev_info(AZ_DEV, "successfully probed device\n");

When drivers are working properly, they should be quiet.

And what is with the AZ_DEV macro mess?

And can you handle more than one device in the system at one time?  I
think your debugfs logic will get really confused.

thanks,

greg k-h